Skip to content

Conversation

@zacw7
Copy link
Member

@zacw7 zacw7 commented Apr 11, 2025

== NO RELEASE NOTE ==

@prestodb-ci prestodb-ci added the from:Meta PR from Meta label Apr 11, 2025
@zacw7 zacw7 marked this pull request as ready for review April 11, 2025 22:56
@zacw7 zacw7 requested a review from a team as a code owner April 11, 2025 22:56
@zacw7 zacw7 requested a review from jaystarshot April 11, 2025 22:56
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.

@zacw7 : Do you have to regenerate presto_protocol for native engine for this as well ? Can you check ?

@zacw7
Copy link
Member Author

zacw7 commented Apr 12, 2025

@zacw7 : Do you have to regenerate presto_protocol for native engine for this as well ? Can you check ?

No we don't have to. It's already part of the protocol.

@aditi-pandit
Copy link
Contributor

@zacw7 : Do you have to regenerate presto_protocol for native engine for this as well ? Can you check ?

No we don't have to. It's already part of the protocol.

@zacw7 : But do the json field names change by any chance ? I would expect the generated code to be slightly different.

@zacw7
Copy link
Member Author

zacw7 commented Apr 12, 2025

@zacw7 : Do you have to regenerate presto_protocol for native engine for this as well ? Can you check ?

No we don't have to. It's already part of the protocol.

@zacw7 : But do the json field names change by any chance ? I would expect the generated code to be slightly different.

This change doesn't introduce anything new. EquiJoinClause is already part of protocol with fields names "left" and "right".

struct EquiJoinClause {
VariableReferenceExpression left = {};
VariableReferenceExpression right = {};
};

The protocol is expecting "left" and "right" while in IndexJoinNode they are currently named as "probe" and "index" which would cause issue.

@zacw7 zacw7 force-pushed the index-join-compatible branch from 208301f to 2b328c7 Compare April 12, 2025 00:31
Copy link
Contributor

@xiaoxmeng xiaoxmeng left a comment

Choose a reason for hiding this comment

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

@zacw7 thanks!

@aditi-pandit
Copy link
Contributor

@zacw7 : Do you have to regenerate presto_protocol for native engine for this as well ? Can you check ?

No we don't have to. It's already part of the protocol.

@zacw7 : But do the json field names change by any chance ? I would expect the generated code to be slightly different.

This change doesn't introduce anything new. EquiJoinClause is already part of protocol with fields names "left" and "right".

struct EquiJoinClause {
VariableReferenceExpression left = {};
VariableReferenceExpression right = {};
};

The protocol is expecting "left" and "right" while in IndexJoinNode they are currently named as "probe" and "index" which would cause issue.

@zacw7 : Thanks for explaining to me. Now your PR title seems more understandable as well.

@zacw7 zacw7 requested a review from NikhilCollooru April 14, 2025 17:00
@zacw7 zacw7 merged commit 411def9 into prestodb:master Apr 14, 2025
97 checks passed
@zacw7 zacw7 deleted the index-join-compatible branch April 14, 2025 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

from:Meta PR from Meta

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants