Skip to content

Comments

[native pos] Fail, not hang, broadcast queries#19676

Merged
mbasmanova merged 2 commits intoprestodb:masterfrom
mbasmanova:fix-broadcast
May 17, 2023
Merged

[native pos] Fail, not hang, broadcast queries#19676
mbasmanova merged 2 commits intoprestodb:masterfrom
mbasmanova:fix-broadcast

Conversation

@mbasmanova
Copy link
Contributor

Broadcast queries used to hang forever. Now these queries fail quickly with a clear error message:

Broadcast shuffle is not supported

Also, check for replicate-nulls-and-any shuffle mode and reject these queries as well.

== NO RELEASE NOTE ==

@mbasmanova mbasmanova requested a review from a team as a code owner May 16, 2023 21:37
@shrinidhijoshi
Copy link
Collaborator

shrinidhijoshi commented May 16, 2023

Isn't it better to implement this check before the query execution starts (AbstractPrestoSparkQueryExecution.java#L785 )?, than during task-execution ?

@mbasmanova
Copy link
Contributor Author

Isn't it better to implement this check before the query execution starts than during task-execution ?

This is a good question. It would be nice to avoid duplicating the logic of what's supported and what is not. One option is to add an API to Prestissimo to check if plan fragment is supported. Then use this API on the driver to check that all fragments are supported before starting query execution.

@shrinidhijoshi
Copy link
Collaborator

That would be ideal that this information lies with prestissimo.
We will have to add it as hardcoded list/java-logic in the presto-native-execution module, given we don't really have any instance of prestissimo worker running during query planning phase.

@mbasmanova
Copy link
Contributor Author

We will have to add it as hardcoded list/java-logic in the presto-native-execution module, given we don't really have any instance of prestissimo worker running during query planning phase.

I think we can make changes to start native process on the driver for the purpose of plan validation. (We can also use it for constant folding of expressions that use external functions.)

@shrinidhijoshi
Copy link
Collaborator

shrinidhijoshi commented May 16, 2023

I am split between just hardcoding the info (simple but hardcoded) of whats currently supported v/s starting a sidecar to evaluate this info (complex but clean)

We can also use it for constant folding of expressions that use external functions.

This might be a strong reason to add sidecar. I don't understand this use-case fully. Is there no other way to accomplish this.

@shrinidhijoshi
Copy link
Collaborator

shrinidhijoshi commented May 17, 2023

Thanks @mbasmanova for explaining constant folding of expressions, for external functions. Looks like we will need Velox library to evaluate these during planning. So makes sense that we have to go with sidecar approach.

Do you want to still merge this PR ? This one might become throwaway work once we implement the sidecar solution.

@mbasmanova
Copy link
Contributor Author

This one might become throwaway work once we implement the sidecar solution.

I think we still need to have proper checks in the C++ process as it cannot rely on the client (Java process) to always behave well.

@shrinidhijoshi shrinidhijoshi self-assigned this May 17, 2023
Copy link
Collaborator

@shrinidhijoshi shrinidhijoshi left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for working on this

@mbasmanova mbasmanova force-pushed the fix-broadcast branch 2 times, most recently from cac4d4d to 80011e2 Compare May 17, 2023 02:35
Broadcast queries used to hang forever. Now these queries fail quickly with a
clear error message:

> Broadcast shuffle is not supported
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.

@mbasmanova LGTM. thanks!

…e mode

This mode is currently not supported. Ignoring this flag may cause hard-to-debug
incorrect results.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants