Skip to content

Conversation

@pdabre12
Copy link
Contributor

@pdabre12 pdabre12 commented Dec 2, 2024

Description

With the introduction of the presto-native-execution module, Presto now has forked the execution engine into a separate process. This allows for the execution engine to be written in C++ and to be more efficient than the Java execution engine. However, this also means that the execution engine code is now entirely separate from the coordinator code, and that the types supported by the coordinator might not necessarily be supported by the execution engine.

This change adds a new native type manager which allows the types supported by the C++ engine to be known to the Java coordinator.

For more context: RFC-0003

Test Plan

Unit and end-to-end tests. More comprehensive end-to-end tests will be written in the future.

Contributor checklist

  • Please make sure your submission complies with our contributing guide, in particular code style and commit standards.
  • PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
  • Documented new properties (with its default value), SQL syntax, functions, or other functionality.
  • If release notes are required, they follow the release notes guidelines.
  • Adequate tests were added if applicable.
  • CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

Prestissimo (Native Execution) Changes
* Add a native type manager

@prestodb-ci prestodb-ci added the from:IBM PR from IBM label Dec 2, 2024
@prestodb-ci prestodb-ci requested review from a team, ScrapCodes and auden-woolfson and removed request for a team December 2, 2024 22:11
@pdabre12 pdabre12 force-pushed the add-native-types-manager branch 3 times, most recently from 85c6d41 to bfb9ee4 Compare December 9, 2024 19:07
@pdabre12 pdabre12 changed the title [WIP] [Do not review] [native] Add a native type manager [native] Add a native type manager Dec 9, 2024
@pdabre12 pdabre12 changed the title [native] Add a native type manager Add a native type manager Dec 9, 2024
@pdabre12 pdabre12 marked this pull request as ready for review December 9, 2024 22:02
@pdabre12 pdabre12 requested a review from presto-oss December 9, 2024 22:02
@pdabre12 pdabre12 force-pushed the add-native-types-manager branch 2 times, most recently from 9ae0d1c to fa3a49b Compare December 10, 2024 20:26
Copy link
Contributor

@auden-woolfson auden-woolfson left a comment

Choose a reason for hiding this comment

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

LGTM

@pdabre12 pdabre12 requested a review from tdcmeehan December 10, 2024 20:49
@pdabre12 pdabre12 force-pushed the add-native-types-manager branch from fa3a49b to cb8ea2d Compare December 10, 2024 22:54
@tdcmeehan tdcmeehan self-assigned this Jan 31, 2025
@pdabre12 pdabre12 force-pushed the add-native-types-manager branch from cb8ea2d to b93cb01 Compare February 4, 2025 22:41
@pdabre12 pdabre12 dismissed tdcmeehan’s stale review March 12, 2025 18:48

Addressed the comments

@pdabre12
Copy link
Contributor Author

@ZacBlanco Do you wanna take a look?

public Type getType(TypeSignature typeSignature)
{
// Todo: Fix this hack, native execution does not support parameterized char/varchar type signatures.
if (typeSignature.getBase().equals(CHAR) || typeSignature.getBase().equals(VARCHAR)) {
Copy link
Contributor

@aditi-pandit aditi-pandit Mar 19, 2025

Choose a reason for hiding this comment

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

Trying to reduce the impact of this hack. What happens if we remove CHAR from this condition and fail all the queries with it ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, removed CHAR.

@pdabre12
Copy link
Contributor Author

pdabre12 commented Mar 20, 2025

@rschlussel
Can you please have another look?
Native engine shouldn't allow creating tables with char column.
Whenever the sidecar plugin is enabled, any queries on the older tables constructed with char column will fail.
Is it safe to do so in your production environment?

@pdabre12 pdabre12 requested a review from rschlussel March 24, 2025 21:36
@libianoss
Copy link

@rschlussel can you help to review this PR?Thanks.

@rschlussel
Copy link
Contributor

@rschlussel Can you please have another look? Native engine shouldn't allow creating tables with char column. Whenever the sidecar plugin is enabled, any queries on the older tables constructed with char column will fail. Is it safe to do so in your production environment?

i don't think i understand what this means. Why doesn't native engine allow creating tables with char column? Currently without sidecar, what happens if someone runs a query on a table with a char column?

@pdabre12
Copy link
Contributor Author

@rschlussel Can you please have another look? Native engine shouldn't allow creating tables with char column. Whenever the sidecar plugin is enabled, any queries on the older tables constructed with char column will fail. Is it safe to do so in your production environment?

i don't think i understand what this means. Why doesn't native engine allow creating tables with char column? Currently without sidecar, what happens if someone runs a query on a table with a char column?

As of now, the native engine allows creating tables with char column and queries on a table with a char column should succeed. But with sidecar enabled, we are getting a list of supported types by the native engine and CHAR is not a part of it. As a result, if you have old tables created with the underlying column type as char and enable the sidecar, the queries on that table will fail.

@rschlussel
Copy link
Contributor

@rschlussel Can you please have another look? Native engine shouldn't allow creating tables with char column. Whenever the sidecar plugin is enabled, any queries on the older tables constructed with char column will fail. Is it safe to do so in your production environment?

i don't think i understand what this means. Why doesn't native engine allow creating tables with char column? Currently without sidecar, what happens if someone runs a query on a table with a char column?

As of now, the native engine allows creating tables with char column and queries on a table with a char column should succeed. But with sidecar enabled, we are getting a list of supported types by the native engine and CHAR is not a part of it. As a result, if you have old tables created with the underlying column type as char and enable the sidecar, the queries on that table will fail.

if currently we can run queries on char columns, then why is it not supported?

@pdabre12
Copy link
Contributor Author

pdabre12 commented Mar 26, 2025

@rschlussel Can you please have another look? Native engine shouldn't allow creating tables with char column. Whenever the sidecar plugin is enabled, any queries on the older tables constructed with char column will fail. Is it safe to do so in your production environment?

i don't think i understand what this means. Why doesn't native engine allow creating tables with char column? Currently without sidecar, what happens if someone runs a query on a table with a char column?

As of now, the native engine allows creating tables with char column and queries on a table with a char column should succeed. But with sidecar enabled, we are getting a list of supported types by the native engine and CHAR is not a part of it. As a result, if you have old tables created with the underlying column type as char and enable the sidecar, the queries on that table will fail.

if currently we can run queries on char columns, then why is it not supported?

I am not really sure what happens under the hood right now when you create a table with char columns in a native cluster. The native engine does not understand char type, maybe it gets casted to a varchar( not sure) ? cc: @aditi-pandit.
But with this change, we only report the types supported by the native engine to the coordinator and as of now char is not one of them, so the queries would fail. This isn't an exclusive case for the char type but all the older and newer tables/queries with types not supported by the native engine would fail.

@rschlussel
Copy link
Contributor

@gggrace14 showed me that we actually are failing now in the workers when you try to query a char column, so this change should be okay.

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 @pdabre12

@pdabre12 pdabre12 merged commit 68cdacd into prestodb:master Mar 26, 2025
95 checks passed
@pdabre12 pdabre12 deleted the add-native-types-manager branch March 26, 2025 19:46
@aditi-pandit
Copy link
Contributor

aditi-pandit commented Mar 26, 2025

@rschlussel Can you please have another look? Native engine shouldn't allow creating tables with char column. Whenever the sidecar plugin is enabled, any queries on the older tables constructed with char column will fail. Is it safe to do so in your production environment?

i don't think i understand what this means. Why doesn't native engine allow creating tables with char column? Currently without sidecar, what happens if someone runs a query on a table with a char column?

As of now, the native engine allows creating tables with char column and queries on a table with a char column should succeed. But with sidecar enabled, we are getting a list of supported types by the native engine and CHAR is not a part of it. As a result, if you have old tables created with the underlying column type as char and enable the sidecar, the queries on that table will fail.

if currently we can run queries on char columns, then why is it not supported?

I am not really sure what happens under the hood right now when you create a table with char columns in a native cluster. The native engine does not understand char type, maybe it gets casted to a varchar( not sure) ? cc: @aditi-pandit. But with this change, we only report the types supported by the native engine to the coordinator and as of now char is not one of them, so the queries would fail. This isn't an exclusive case for the char type but all the older and newer tables/queries with types not supported by the native engine would fail.

@rschlussel : Today C++ clusters would support CREATE TABLE with char columns (as that is purely at the co-ordinator), but SELECT from them would fail at the workers since the PrestoToVelox conversions would reject the CHAR column. With this change, CHAR would not be exposed as a type, so even CREATE TABLE with char columns would fail.

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.