Skip to content

[native]Add TableWriterMergeNode in presto protocol#20046

Merged
xiaoxmeng merged 1 commit intoprestodb:masterfrom
xiaoxmeng:merge
Jul 5, 2023
Merged

[native]Add TableWriterMergeNode in presto protocol#20046
xiaoxmeng merged 1 commit intoprestodb:masterfrom
xiaoxmeng:merge

Conversation

@xiaoxmeng
Copy link
Contributor

@xiaoxmeng xiaoxmeng commented Jul 4, 2023 

== NO RELEASE NOTE ==

@xiaoxmeng xiaoxmeng changed the title [native]Add table writer merge node in presto protocol [native]Add TableWriterMergeNode in presto protocol Jul 4, 2023
@xiaoxmeng xiaoxmeng marked this pull request as ready for review July 4, 2023 06:28
@xiaoxmeng xiaoxmeng requested a review from a team as a code owner July 4, 2023 06:28
yingsu00
yingsu00 reviewed Jul 4, 2023
View reviewed changes
presto-native-execution/presto_cpp/presto_protocol/presto_protocol.h Outdated
Copy link
Contributor

@yingsu00 yingsu00 Jul 4, 2023

Choose a reason for hiding this comment

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

There are so many formatting changes and it's difficult to see what's actually changed. Would it be possible to exclude the pure formatting changes?

Copy link
Contributor

@mbasmanova mbasmanova Jul 4, 2023

Choose a reason for hiding this comment

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

@yingsu00 These files are auto-generated by running make protocol.

mbasmanova
mbasmanova approved these changes Jul 4, 2023
View reviewed changes
Copy link
Contributor

@mbasmanova mbasmanova left a comment

Choose a reason for hiding this comment

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

@xiaoxmeng Thanks.

@mbasmanova
Copy link
Contributor

mbasmanova commented Jul 4, 2023

@xiaoxmeng There are CI failures:

[ERROR] Failures: 
[ERROR]   TestPrestoNativeAggregations>AbstractTestQueryFramework.init:84->createExpectedQueryRunner:31 ? Creation Unable to create injector, see the following errors:

1) Configuration property 'hive.allow-drop-table' was not used
  at com.facebook.airlift.bootstrap.Bootstrap.lambda$initialize$2(Bootstrap.java:244)

1 error
[ERROR]   TestPrestoNativeJoinQueries>AbstractTestQueryFramework.init:84->createExpectedQueryRunner:31 ? Creation Unable to create injector, see the following errors:

1) Configuration property 'hive.allow-drop-table' was not used
  at com.facebook.airlift.bootstrap.Bootstrap.lambda$initia

@xiaoxmeng
Copy link
Contributor Author

xiaoxmeng commented Jul 4, 2023

@mbasmanova thanks for the quick review. Let me rebase and the hive drop table option change has been reverted.

@amitkdutta
Copy link
Contributor

amitkdutta commented Jul 4, 2023

@mbasmanova thanks for the quick review. Let me rebase and the hive drop table option change has been reverted.

@xiaoxmeng There are CI failures:

[ERROR] Failures: 
[ERROR]   TestPrestoNativeAggregations>AbstractTestQueryFramework.init:84->createExpectedQueryRunner:31 ? Creation Unable to create injector, see the following errors:

1) Configuration property 'hive.allow-drop-table' was not used
  at com.facebook.airlift.bootstrap.Bootstrap.lambda$initialize$2(Bootstrap.java:244)

1 error
[ERROR]   TestPrestoNativeJoinQueries>AbstractTestQueryFramework.init:84->createExpectedQueryRunner:31 ? Creation Unable to create injector, see the following errors:

1) Configuration property 'hive.allow-drop-table' was not used
  at com.facebook.airlift.bootstrap.Bootstrap.lambda$initia

Its from a local master. The allow-drop-table option is reverted #20025

@xiaoxmeng xiaoxmeng merged commit 4d1bb73 into prestodb:master Jul 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yingsu00 yingsu00 yingsu00 left review comments

@mbasmanova mbasmanova mbasmanova approved these changes

@amitkdutta amitkdutta Awaiting requested review from amitkdutta

@kewang1024 kewang1024 Awaiting requested review from kewang1024

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@xiaoxmeng @mbasmanova @amitkdutta @yingsu00