Conversation
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?
There was a problem hiding this comment.
Logged https://issues.apache.org/jira/browse/HIVE-30070 and replaced comment with JIRA reference: c000403
| 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?
There was a problem hiding this comment.
For the moment yes! My personal repo is also used in: standalone-metastore/metastore-server/docker/hive-postgres-tpcds-metastore/Dockerfile
Once I find time, I will try to migrate those to the apache namespace as per discussion: https://lists.apache.org/thread/wdmnhk1tg42vxbpsjz1jc309l2shmr23
| 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?
There was a problem hiding this comment.
This runs with the 10TB dataset so I fixed the typos: 0399773
| 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)?
There was a problem hiding this comment.
Good point. I simplified a lot this class in f8c64aa
| 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?
There was a problem hiding this comment.
Fixed e35b69d
I have the impression that all cleanup scripts are redundant because every test runs in its own JVM so the whole process comes down after the execution.
- 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
Even if the tmp file is not deleted by the JVM it will be cleaned during the maven clean lifecycle.
2991e5e to
c000403
Compare
|
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?