Skip to content

Refactor assertCreateTableAsSelect() and assertExplainAnalyze() code#21426

Closed
pdabre12 wants to merge 1 commit intoprestodb:masterfrom
pdabre12:refactor-CTAS-code
Closed

Refactor assertCreateTableAsSelect() and assertExplainAnalyze() code#21426
pdabre12 wants to merge 1 commit intoprestodb:masterfrom
pdabre12:refactor-CTAS-code

Conversation

@pdabre12
Copy link
Contributor

Refactored the assertCreateTableAsSelect() and assertExplainAnalyze() to reduce code duplication.

@pdabre12 pdabre12 changed the title Refactored assertCreateTableAsSelect() and assertExplainAnalyze() code Refactor assertCreateTableAsSelect() and assertExplainAnalyze() code Nov 21, 2023
@pdabre12 pdabre12 marked this pull request as ready for review November 21, 2023 23:40
@pdabre12 pdabre12 requested a review from a team as a code owner November 21, 2023 23:40
@pdabre12 pdabre12 requested review from majetideepak and presto-oss and removed request for presto-oss November 21, 2023 23:40
@pdabre12
Copy link
Contributor Author

@majetideepak Please review.

@majetideepak
Copy link
Collaborator

@pdabre12 Can you give a motivation for this refactor? Do we ever call assertCreateTableAsSelect with allowDropTable set to false?

@pdabre12
Copy link
Contributor Author

@majetideepak Yes, it will be used in #20936.

@majetideepak
Copy link
Collaborator

@majetideepak Yes, it will be used in #20936.

Why do we not want to delete the tables in that PR?

@pdabre12
Copy link
Contributor Author

pdabre12 commented Dec 15, 2023

There is a small discussion about this in PR #20936.
Basically in order to drop tables, I need to set a system property hive.allow-drop-table=true which would mean modifying the file https://github.com/prestodb/presto/blob/dd5b168ff5bdf306de70be19cc37c9797ca00981/presto-native-execution/src/test/java/com/facebook/presto/nativeworker/NativeQueryRunnerUtils.java .

@majetideepak
Copy link
Collaborator

There is a small discussion about this in PR #20936.

I commented there. Let's add the system property to the NativeQueryRunner in a separate PR.
I don't think we should allow not dropping the tables.

@pdabre12
Copy link
Contributor Author

pdabre12 commented Jan 9, 2024

@majetideepak
Added the changes in #21641.
Closing this PR as it's no longer needed.

@pdabre12 pdabre12 closed this Jan 9, 2024
@pdabre12 pdabre12 deleted the refactor-CTAS-code branch March 1, 2024 20:39
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.

2 participants