[CALCITE-7541] Support Binary Arrow types#4957
Conversation
| /** | ||
| * Enumerator that reads projected Arrow value-vectors directly. | ||
| */ | ||
| class ArrowDirectEnumerator extends AbstractArrowEnumerator { |
There was a problem hiding this comment.
Is this related to the issue being addressed?
There was a problem hiding this comment.
Yes, it is related. The existing projection path goes through Gandiva Projector, but Gandiva does not handle these Arrow binary vectors well in this case.
For this PR, the direct enumerator is only used for simple field projection without filters. It reads the selected Arrow vectors directly instead of asking Gandiva to project them. I added comments to make that scope clearer.
There was a problem hiding this comment.
I thought you want to get rid of Gandiva.
Do you still want to keep this new comment?
Will it be removed later?
There was a problem hiding this comment.
I will do this in another pull request. After merging this pull request, I will pull the latest branch code for modification.
| } catch (GandivaException e) { | ||
| throw Util.toUnchecked(e); | ||
| if (containsGandivaUnsupportedField(fields)) { | ||
| projector = null; |
There was a problem hiding this comment.
Can you explain why is it ok for the projector to be null?
There was a problem hiding this comment.
It is intentional here. When there is no filter and the selected fields need direct vector projection, projector is left null and ArrowEnumerable falls back to ArrowDirectEnumerator.
So the cases are now:
- projector: Gandiva projection
- filter: Gandiva filter
- neither: direct vector projection
I renamed the helper and added comments to make this contract explicit.
caicancai
force-pushed
the
calcite-7541-binary-arrow-types
branch
from
b8633a7 to
8f05381
Compare
mihaibudiu
left a comment
mihaibudiu
left a comment
There was a problem hiding this comment.
Since you want to get rid of Gandiva, does this PR still make sense?
| /** | ||
| * Enumerator that reads projected Arrow value-vectors directly. | ||
| */ | ||
| class ArrowDirectEnumerator extends AbstractArrowEnumerator { |
There was a problem hiding this comment.
I thought you want to get rid of Gandiva.
Do you still want to keep this new comment?
Will it be removed later?
|
|
||
| private boolean containsListField(ImmutableIntList fields) { | ||
| /** Returns whether selected fields should be projected by reading Arrow | ||
| * value-vectors directly rather than by creating a Gandiva projector. |
caicancai
added
the
LGTM-will-merge-soon
|
I have approved, so feel free to merge. |
caicancai
force-pushed
the
calcite-7541-binary-arrow-types
branch
from
8f05381 to
a36f41c
Compare
|
@mihaibudiu thank you for your review |
|
2 participants
Jira Link
CALCITE-7541
Changes Proposed