Skip to content

[native] Add test for substring alias #25197

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

yabinma
Copy link
Member

@yabinma yabinma commented May 26, 2025

== NO RELEASE NOTE ==

@yabinma yabinma requested a review from a team as a code owner May 26, 2025 07:42
@prestodb-ci prestodb-ci added the from:IBM PR from IBM label May 26, 2025
@prestodb-ci prestodb-ci requested review from a team, bibith4 and wanglinsong and removed request for a team May 26, 2025 07:42
@yabinma yabinma changed the title Add test for substring alias [native]Add test for substring alias May 26, 2025
Copy link
Contributor

@aditi-pandit aditi-pandit left a comment

Choose a reason for hiding this comment

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

Thanks @yabinma

Copy link
Contributor

@aditi-pandit aditi-pandit left a comment

Choose a reason for hiding this comment

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

@yabinma : Please fix the failing tests... Think that is because presto-teradata-functions module is not registered on the Java side. Please can you check.

@yabinma yabinma requested a review from a team as a code owner May 30, 2025 10:44
@yabinma yabinma requested a review from jaystarshot May 30, 2025 10:44
@yabinma
Copy link
Member Author

yabinma commented May 30, 2025

@yabinma : Please fix the failing tests... Think that is because presto-teradata-functions module is not registered on the Java side. Please can you check.

Got a chance to check today. substring is already registered in presto-teradata-functions,

I was thinking it might be missed in common place, that's why I added another substring registration in com.facebook.presto.operator.scalar.StringFunctions
But. the workflow failed with error message,

25/05/30 06:03:17 WARN PluginManagerUtil: No service providers of type com.facebook.presto.spi.RouterPlugin
25/05/30 06:03:17 WARN PluginManagerUtil: No service providers of type com.facebook.presto.spi.CoordinatorPlugin
25/05/30 06:03:17 INFO PluginManagerUtil: Installing com.facebook.presto.teradata.functions.TeradataFunctionsPlugin
25/05/30 06:03:17 INFO PluginManager: Registering functions from com.facebook.presto.teradata.functions.TeradataDateFunctions
25/05/30 06:03:17 INFO PluginManager: Registering functions from com.facebook.presto.teradata.functions.TeradataStringFunctions
java.lang.RuntimeException: java.lang.IllegalArgumentException: Function already registered: presto.default.substring(varchar(x),bigint,bigint):varchar(x)

So the previous "substring is not registered" error might not be caused by function register code. I will check the workflow later how the test server is configured. It might need to add the TeradataStringFunctions registration in the configuration.

@yabinma
Copy link
Member Author

yabinma commented May 30, 2025

I rollback the code to contain the test case changes only and the workflow returned the same error message that "substring is not registered"

It failed in step: prestocpp-linux-presto-e2e-tests: then I checked the workflow code in https://github.com/prestodb/presto/blob/fda51140321fca27f8054786c3ceedd555700cb5/.github/workflows/prestocpp-linux-build-and-unit-test.yml#L93C3-L93C36

The dependency image shown: container:
image: prestodb/presto-native-dependency:0.293-20250522140509-484b00e

My Velox PR was merged in 20250523. This might be one of the reasons.

@aditi-pandit , do you know how frequently the test image is updated?

@amitkdutta amitkdutta changed the title [native]Add test for substring alias [native] Add test for substring alias May 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
from:IBM PR from IBM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants