Conversation
- Add TPCDS planner tests for Iceberg tables backed by S3 container - Add S3Container using rustfs for tests that need s3a:// access - Add test driver, init script, and expected output files for all 99 TPCDS queries
asf-ci-hive
added
tests pending
tests unstable
and removed
tests pending
tests unstable
labels
|
|
The new tests took ~8 minutes to run in CI: https://ci.hive.apache.org/job/hive-precommit/job/PR-6781/2/testReport/org.apache.hadoop.hive.cli/TestTPCDSIcebergCliDriver/ |
| conf.put(HiveConf.ConfVars.PRE_EXEC_HOOKS, ""); | ||
| conf.put(HiveConf.ConfVars.POST_EXEC_HOOKS, ""); | ||
| // Disable merge join conversion cause it triggers very slow S3 list | ||
| // operations. Check to reenable later |
There was a problem hiding this comment.
nit: "Check to reenable later" maybe worth a TODO here?
| setCleanupScript("q_test_cleanup_tez.sql"); | ||
| S3Container.BucketSpec bucketSpec = new S3Container.BucketSpec( | ||
| "dw-team-bucket/data/warehouse/tablespace/external/hive/tpcds_partitioned_iceberg_parquet_10000.db", | ||
| "https://github.com/zabetak/hive-test-datasets/releases/download/1.1/iceberg_s3_tpcds10tb.zip"); |
There was a problem hiding this comment.
is it ok to have a personal repo here?
| public TPCDSIcebergS3CliConfig() { | ||
| super(CorePerfCliDriver.class); | ||
| setQueryDir("ql/src/test/queries/clientpositive/perf"); | ||
| setLogDir("itests/qtest/target/qfile-results/clientpositive/perf/tpcds1tb/iceberg"); |
There was a problem hiding this comment.
LogDir and ResultsDir say ".../tpcds1tb/..." (1TB); but below, the bucketSpec is tpcds_partitioned_iceberg_parquet_10000.db and iceberg_s3_tpcds10tb.zip (10TB); is this an inconsistency?
| String bucketName; | ||
| URL dataUrl; | ||
|
|
||
| public BucketSpec(String bucketName, String dataUrl) { |
There was a problem hiding this comment.
nit: why not simply passing bucket and keyPrefix as constructor arguments (and have them as separate final fields), so that we wouldn't need the extra substr logic on the getters (we wouldn't need the getters at all in fact)?
| testArguments.setCleanupScript(cleanupScript); | ||
| testArguments.setWithLlapIo(withLlapIo); | ||
|
|
||
| testArguments.s3BucketSpec = this.s3BucketSpec; |
There was a problem hiding this comment.
nit: for consistency with the existing code, this should be done via a setter.
|
|
||
| private void setupS3(HiveConf conf) { | ||
| S3Container.BucketSpec bucket = testArgs.getS3BucketSpec(); | ||
| if(bucket == null) { |
There was a problem hiding this comment.
nit: missing space after if
|
|
||
| private static final Logger LOG = LoggerFactory.getLogger(S3Container.class); | ||
| private static final DockerImageName RUSTFS_IMAGE = | ||
| DockerImageName.parse("rustfs/rustfs:v1.0.0-rc.5-glibc"); |
There was a problem hiding this comment.
Could we use 1.0.0 (which seems it was released just yesterday: https://github.com/rustfs/rustfs/releases#release-1.0.0) instead of a RC?
| setResultsDir("ql/src/test/results/clientpositive/perf/tpcds1tb/iceberg"); | ||
|
|
||
| setInitScript("q_init_tpcds_iceberg.sql"); | ||
| setCleanupScript("q_test_cleanup_tez.sql"); |
There was a problem hiding this comment.
just double-checking: is this existing cleanup script sufficient for this new test case?
3 participants
What changes were proposed in this pull request?
Why are the changes needed?
For catching performance and functionality regressions of the query planner for Iceberg tables.
Does this PR introduce any user-facing change?
No
How was this patch tested?