-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
feat: Support Spark explode outer #11954
base: main
Are you sure you want to change the base?
feat: Support Spark explode outer #11954
Conversation
✅ Deploy Preview for meta-velox ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
@pedroerp @rui-mo @jinchengchenghh @PHILO-HE Can you please help review this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
velox/exec/tests/UnnestTest.cpp
Outdated
// is_outer = false | ||
auto op1 = PlanBuilder().values({vector}).unnest({"c0"}, {"c1"}, std::nullopt, false /* is_outer */).planNode(); | ||
auto params1 = makeCursorParameters(op1); | ||
auto expected1 = makeRowVector({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can we have 'op', 'params', 'expected' and reuse them in the other test cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can we have 'op', 'params', 'expected' and reuse them in the other test cases?
just follow the style of other test cases in UnnestTest
. Maybe another PR to refactor it?
velox/exec/tests/UnnestTest.cpp
Outdated
|
||
createDuckDbTable({vector}); | ||
|
||
// is_outer = false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: isOuter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution. Overall looks good. % minors.
I don't think we need to use isOuter as template, isOuter is not a too expensive cost, It will make the change not precise. What do you think? @rui-mo
if (rawOrdinalityIsNull_[row]) { | ||
ordinalityVector->setNull(index, true); | ||
} | ||
} | ||
std::iota(rawOrdinality, rawOrdinality + size, start + 1); | ||
rawOrdinality += size; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then we don't need to move rawOrdinality, just use rawOrdinality + index
in L263.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you mean change it to std::iota(rawOrdinality + index, rawOrdinality + index + size, start + 1);
right?
Support the explode/posexplode with outer semantic. Unlike explode/posexplode, if the array/map is null or empty then null is produced.
Spark doc: https://spark.apache.org/docs/latest/api/python/reference/pyspark.sql/api/pyspark.sql.functions.explode_outer.html