-
Notifications
You must be signed in to change notification settings - Fork 5.5k
[native] Advance velox #25318
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
[native] Advance velox #25318
Conversation
amitkdutta
commented
Jun 13, 2025
|
Noisy aggregation issue is resolved. Current problem is protocol. |
|
@amitkdutta : There is a crash in FAILED: presto_cpp/main/types/tests/presto_to_velox_query_plan_test... We'll have to investigate this. I opened #25324 |
|
@aditi-pandit Thanks for looking in the unit test issue. I see a protocol issue also Not sure what changed in the protocol. |
|
@amitkdutta : That's a good signal. It not the same symbols in the test failures, but its mostly a protocol related issue than a connector or iceberg specific one. |
|
@aditi-pandit The weird thing is that, taking the first error on the Lifespan reference, I don't see Lifespan references on line 698 of presto_protocol_core.h: https://github.com/prestodb/presto/blob/master/presto-native-execution/presto_cpp/presto_protocol/core/presto_protocol_core.h#L698 It's only defined on line 730 and referenced below. Looking at the code referenced in the error, it could be from line 755: https://github.com/prestodb/presto/blob/master/presto-native-execution/presto_cpp/presto_protocol/core/presto_protocol_core.h#L698 but that is after the struct definition. |
|
CC: @czentgr @imjalpreet |
|
@aditi-pandit Since this is not blocking willing to merge because new velox have some errors that will be needed quickly by @singcha . Opening an issue to track protocol problem |
|
@amitkdutta If we are willing to break the prestissimo build for a while then I suppose we can merge it. So we have two issues. more more urgent than the other. The symbols issue blocks any prestissimo testing. The protocol issue is likely of less importance right now compared to the symbol issue. |
@amitkdutta : Agree with Christian... I initially thought this was something iceberg specific... but on closer inspection it looks much deeper than that... Absolutely no prestissimo tests have run with this build. Thats not good. |
aditi-pandit
left a comment
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.
Putting a hold until we resolve the test failures.
|
@czentgr : There are tons of changes in CmakeLists, other build artifacts in the new Velox code added in this PR. Given the test failure I saw, I initially thought that this commit could be problematic... But any other clues ? |
|
@aditi-pandit I have a fix for the symbol issue. Not sure exactly why, but explicitly adding the velox hive connector and tpch connector to the dependency list works. The symbols are in these libraries but somehow not added to the executable otherwise. I've told @amitkdutta what to patch. So it can be done in his PR or we can merge the Velox update PR and I can make a separate fix PR. |
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 Christian.
Removing hold to avoid this staying over the weekend.
As per @czentgr last comment, we can patch later if needed
|
Superseded by #25326. |