[native] Use builder pattern for native queryRunner#25120
[native] Use builder pattern for native queryRunner#25120pramodsatya merged 1 commit intoprestodb:masterfrom
Conversation
pramodsatya
force-pushed
the
bldr_nativeqr
branch
3 times, most recently
from
9d9e72c to
c8c9a74
Compare
prestodb-ci
requested review from
a team,
BryanCutler and
infvg
and removed request for
a team
|
Thank you for working on this! |
...e-execution/src/test/java/com/facebook/presto/nativeworker/PrestoNativeQueryRunnerUtils.java
Outdated
Show resolved
Hide resolved
...e-execution/src/test/java/com/facebook/presto/nativeworker/PrestoNativeQueryRunnerUtils.java
Outdated
Show resolved
Hide resolved
...e-execution/src/test/java/com/facebook/presto/nativeworker/PrestoNativeQueryRunnerUtils.java
Outdated
Show resolved
Hide resolved
pramodsatya
force-pushed
the
bldr_nativeqr
branch
3 times, most recently
from
47517a3 to
89564d4
Compare
|
Thanks for the feedback @tdcmeehan, @ZacBlanco, @aditi-pandit, addressed the review comments. Could you please take another look? |
ZacBlanco
left a comment
ZacBlanco
left a comment
There was a problem hiding this comment.
One comment, otherwise lgtm
...e-execution/src/test/java/com/facebook/presto/nativeworker/PrestoNativeQueryRunnerUtils.java
Outdated
Show resolved
Hide resolved
BryanCutler
left a comment
BryanCutler
left a comment
There was a problem hiding this comment.
LGTM, just a couple minor nits
...tion/src/test/java/com/facebook/presto/nativeworker/TestPrestoNativeCteExecutionParquet.java
Outdated
Show resolved
Hide resolved
...ve-tests/src/test/java/com/facebook/presto/nativetests/TestDistributedEngineOnlyQueries.java
Outdated
Show resolved
Hide resolved
presto-native-tests/src/test/java/com/facebook/presto/nativetests/TestOrderByQueries.java
Outdated
Show resolved
Hide resolved
presto-native-tests/src/test/java/com/facebook/presto/nativetests/TestRepartitionQueries.java
Outdated
Show resolved
Hide resolved
...ests/src/test/java/com/facebook/presto/nativetests/TestRepartitionQueriesWithSmallPages.java
Outdated
Show resolved
Hide resolved
presto-native-tests/src/test/java/com/facebook/presto/nativetests/TestWindowQueries.java
Outdated
Show resolved
Hide resolved
aditi-pandit
left a comment
aditi-pandit
left a comment
There was a problem hiding this comment.
Thanks @pramodsatya. Have one comment, else this PR is looking good.
...tion/src/test/java/com/facebook/presto/nativeworker/TestPrestoNativeCteExecutionParquet.java
Outdated
Show resolved
Hide resolved
...e-execution/src/test/java/com/facebook/presto/nativeworker/PrestoNativeQueryRunnerUtils.java
Outdated
Show resolved
Hide resolved
...e-execution/src/test/java/com/facebook/presto/nativeworker/PrestoNativeQueryRunnerUtils.java
Outdated
Show resolved
Hide resolved
...e-execution/src/test/java/com/facebook/presto/nativeworker/PrestoNativeQueryRunnerUtils.java
Show resolved
Hide resolved
pramodsatya
force-pushed
the
bldr_nativeqr
branch
2 times, most recently
from
93693c8 to
675f276
Compare
|
Thanks @aditi-pandit, @BryanCutler, @ZacBlanco, addressed the pending comments, could you please take another look? |
| .put("hive.storage-format", this.storageFormat) | ||
| .put("hive.pushdown-filter-enabled", "true") | ||
| .build()); | ||
| this.tpcdsProperties.putAll(getNativeWorkerTpcdsProperties()); |
There was a problem hiding this comment.
Its a bit odd this is getNativeWorkerTpcdsProperties as well. Please can you confirm.
You could write this code to move the common properties for the query runner outside of the if condition.
There was a problem hiding this comment.
getNativeWorkerTpcdsProperties() has config tpcds.use-varchar-type=true, which is needed in the java query runner to create Tpcds tables with Varchar columns for the native Tpcds e2e tests.
Moved the common properties outside the if condition, thanks for the suggestion! Could you please take another look?
pramodsatya
force-pushed
the
bldr_nativeqr
branch
from
675f276 to
f0fe21e
Compare
aditi-pandit
left a comment
aditi-pandit
left a comment
There was a problem hiding this comment.
Thanks @pramodsatya
|
@tdcmeehan, could you please take another look? |
6 participants
Description
Consolidate various methods in
PrestoNativeQueryRunnerUtilsused to create query runners for native e2e tests using builder pattern.Motivation and Context
PrestoNativeQueryRunnerUtilscurrently has multiple overloaded methods to create (native | java) (hive | iceberg) query runner using different arguments. All of these overloaded methods use a common function to create the query runner, while allowing developers to modify a set of configs via function arguments and retaining the default values for remaining configs. Moving these parameters to a builder pattern, following the example ofIcebergQueryRunner, will consolidate the differentcreateQueryRunnermethods and improve readability for the configs being set in different test suites.