-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
ARROW-15726: [C++] If a projected_schema is not supplied but a bound projection expression is then we should use that to infer the projected_schema #12466
Conversation
… missing projection or projected_schema fields
Thanks for opening a pull request! If this is not a minor PR. Could you open an issue for this pull request on JIRA? https://issues.apache.org/jira/browse/ARROW Opening JIRAs ahead of time contributes to the Openness of the Apache Arrow project. Then could you also rename pull request title in the following format?
or
See also: |
@ursabot please benchmark lang=R |
Benchmark runs are scheduled for baseline = ade1027 and contender = 5f6b483. Results will be available as each benchmark for each run completes. |
It's not super easy to find, but the logs from the benchmark: https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/159#6bba5997-49a2-4405-b8ae-6172b8a93a1a/6-3467 Are showing the same error we were seeing before:
|
@jonkeane I think you are looking at the wrong conbench run. The run requested for this PR completed successfully: https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/160 |
// Either the expression for this field is not a field_ref or it is not a | ||
// simple field_ref. User must supply projected_schema | ||
return Status::Invalid( | ||
"No projected schema was supplied and we could not infer the projected " | ||
"schema from the projection expression."); |
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.
We could infer names/types by just stringifying the expression and using the expression's type in this case, though I guess as a temporary fix it may not be worth it.
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.
Agreed. There is more we could do here but in general we can leave that up to the caller. If there is a need to use more complex expressions then the caller can supply the projected_schema
and it should work just fine.
|
Which benchmarks are we looking at here? I don't see any meaningful improvements. |
You're totally right, the correct build is 160 which does have dataset benchmarks running just fine. But now I'm confused about why the comment up above that marks the benchmarks as failed:
and if you follow that link to conbench there aren't any dataset benchmarks reported. But both of those are likely conbench issues that don't have anything to do with the PR here. Sorry for the confusion! |
@lidavidm There aren't supposed to be any improvements. An earlier change broke some accidental inference of the projected schema and so conbench was failing. This fix is to get conbench passing again with some intentional inference. |
Ah, I misunderstood. Thanks for the clarification. |
Benchmark runs are scheduled for baseline = 6b63103 and contender = bdd6bcd. bdd6bcd is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
New rules. This is somewhat of a short term fix until we address ARROW-12311.