Substrait consumer build fix #2
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…d of use hard coded filenames. If no filename is specified we return 0 which is needed because CI will call the examples and we cannot fail. Removed the filter from the example expression because it triggers ARROW-15658 and we need to resolve ARROW-15586 first anyways. Added logic in the scanner to provide a default projection when none is specified.
|
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: |
jvanstraten
approved these changes
Feb 11, 2022
|
Thanks for taking on the build/link issue! Ah yeah, I knew the example didn't run without setting up some context first, and since (I thought) CI doesn't run the examples and its reason for failing is fairly obvious I figured it was good enough for now. w.r.t. the projection logic, do you mean that it is implemented by the scanners after all, or just that something was crashing when it was left undefined/null? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Well, after banging my head against the wall trying different things I managed to discover what I think is the problem. Luckily the fix is a 1 word fix.
The cmake flag
LIBPROTOBUF_EXPORTSappears to no longer be required. I think the default now is to always export symbols. However, if you specifyLIBPROTOBUF_EXPORTSand you do not specifyPROTOBUF_USE_DLLSit will actually remove the exporting. Based on this comment I'm thinking the better approach is to specify neither flag.Once that was resolved then the Windows build still failed because it runs the examples and the example failed. I had to touch up a few things to get the example passing. It turns out there was an Arrow bug interfering with filter pushdown (ARROW-12311) so I removed the filter. Also, the files probably won't exist so I changed the example to take in a parquet filename. I made it fake-pass (as we do in some other examples) if no filename is specified. Also, my earlier statement (scan_options.projection is never used) turned out to be not quite accurate. Rather than add that projection logic back in I fixed the scanner to handle the case where no projection is specified.