Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix array_transform to not recompute the argument #10015

Merged
merged 2 commits into from
Dec 12, 2023

Conversation

revans2
Copy link
Collaborator

@revans2 revans2 commented Dec 11, 2023

This fixes #9472

Thanks to @razajafri for doing the majority of the debugging on this.

Signed-off-by: Robert (Bobby) Evans <bobby@apache.org>
@revans2 revans2 self-assigned this Dec 11, 2023
@revans2
Copy link
Collaborator Author

revans2 commented Dec 11, 2023

build

1 similar comment
@revans2
Copy link
Collaborator Author

revans2 commented Dec 11, 2023

build

razajafri
razajafri previously approved these changes Dec 11, 2023
Copy link
Collaborator

@razajafri razajafri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. other than a nit

Comment on lines 232 to 238
val explodedTable = withResource(GpuProjectExec.project(inputBatch, boundIntermediate)) {
intermediateBatch =>
withResource(GpuColumnVector.combineColumns(intermediateBatch, argColumn)) {
projectedBatch =>
withResource(GpuColumnVector.from(projectedBatch)) { projectedTable =>
projectedTable.explodePosition(boundIntermediate.length)
}
Copy link
Collaborator

@razajafri razajafri Dec 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: could this and should this be refactored to a private method? It can be reused by passing the method to run explodePosition or explode

jlowe
jlowe previously approved these changes Dec 11, 2023
@@ -895,6 +895,22 @@ public static ColumnarBatch combineColumns(ColumnarBatch ... batches) {
return incRefCounts(ret);
}

public static ColumnarBatch combineColumns(ColumnarBatch cb, GpuColumnVector ... vectors) {
final int numRows = cb.numRows();
ArrayList<ColumnVector> columns = new ArrayList<>();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: We know exactly how big the array needs to be and can hint this here.

Suggested change
ArrayList<ColumnVector> columns = new ArrayList<>();
ArrayList<ColumnVector> columns = new ArrayList<>(cb.numCols() + vectors.length);

@revans2 revans2 dismissed stale reviews from jlowe and razajafri via 75c91ca December 11, 2023 21:53
@revans2
Copy link
Collaborator Author

revans2 commented Dec 11, 2023

build

@revans2
Copy link
Collaborator Author

revans2 commented Dec 11, 2023

@razajafri and @jlowe please take another look

@razajafri razajafri merged commit aa61be4 into NVIDIA:branch-24.02 Dec 12, 2023
38 checks passed
@sameerz sameerz added the bug Something isn't working label Dec 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Non-Deterministic expressions in an array_transform can cause errors
4 participants