[thomasrebele]

What changes were proposed in this pull request?

Update hive-postgres-tpcds-metastore to use a TPC-DS 30TB dump with histogram column statistics. I've spent quite a lot of time to find a way to reduce the image size while keeping the performance of TestTezTPCDS30TBPerfCliDriver. I opted for compressing the raw database files from Postgres and setting some Postgres config options. See the scripts that I used to cleanup and generate the raw dump.

On my machine, TestTezTPCDS30TBPerfCliDriver using the proposed image runs in about 270s, while TestTezTPCDS30TBPerfCliDriver with the currently used image takes 350s, assuming the images have already been downloaded. So while the image with the histogram column statistics has a much larger database, the tests are executed faster. The download time is not taken into account, as here the image without the histogram statistics would clearly win.

Why are the changes needed?

The dump includes histogram statistics, making it possible to use them in future tests. (HIVE-26830).

Does this PR introduce any user-facing change?

No

How was this patch tested?

I build the image locally and executed TestTezTPCDS30TBPerfCliDriver. See the corresponding q.out file changes. See this comment for an explanation for the q.out file changes (the histograms are not yet used in the planning phase).

[thomasrebele]

As far as I know, the changes of this PR do not affect the tests, so I cancelled continuous-integration/jenkins/pr-head.

[zabetak]

I tried to test the new image with TestTezTPCDS30TBPerfCliDriver but bumped into various errors. In order to make this PR more self-contained it would be nice to include all the necessary changes and have TestTezTPCDS30TBPerfCliDriver run with the new image. For the time being we don't need to switch to use histograms in the test.

The description mentions that tests are going to run faster when image is present but in the current state of the CI we are gonna fetch it everytime so we have to account for an extra 1GB of download overhead.

[zabetak]

Since now we have an official ASF repo for hosting datasets (https://github.com/apache/hive-test-datasets) please raise a PR there for adding the extra details and replace the link to point there.

[zabetak]

I guess we have to update the link since the dump that we use is now in https://nightlies.apache.org/. A pending question is where to put extra details and scripts to avoid linking to personal GitHub accounts but we could settle this later.

[thomasrebele]

Thank you for the review, @zabetak! The new dump changes lots of the q.out files, so I thought updating the image first and then the test files would be appropriate. I'm fine with merging these two steps, but I think I don't have enough rights to upload the files to Apache's Docker hub. As long as the image has not been published, the CI won't be able to run.

I was wondering, whether to include the scripts that I used to generate the dump here. Wdyt?

[zabetak]

@thomasrebele For the first CI run you could use the image from your personal Dockerhub account. If everything comes green I can push the image manually to the Apache DockerHub account and retrigger CI.

I understand the intention of committing the .q.out files and other changes separately but by doing so we are creating a new image that will land untested and will be unused for a certain time. For this reason, I am leaning more towards a slightly more complex and complete PR that starts using the image immediately.

The scripts for creating the dump should reside in the same repo where the dump lives. Let's keep them there for the time being.

[thomasrebele]

Thank you for the suggestions. I'll work on them. I had looked into the options for reducing the image size that you had mentioned. Option 2 for removing the entry point did not work for me.

Question before integrating the q.out file changes into the PR: it came to my mind that the schema migration script could be integrated into the raw DB dump, so that no initialization is necessary. Wdyt, @zabetak?

[zabetak]

The option 2 was not so much for reducing the image size but mainly for skipping the custom entry point and the decompression logic during startup. Not sure if its better but shared it so you can pick whatever you feel best. I enriched the respective discussion with the exact commands and tools I used.

Indeed the schema migration script could be incorporated in the dump but then we may need different images and binary dump files for different versions (e.g., upstream vs downstream). Feel free to pick the option that suits best, I don't have a strong opinion which direction to go.

[thomasrebele]

Thinking more about it, schema changes will require a schema migration script anyway, so integrating it now would only help for a little while.

I've updated the dump to fix the WAL issues, uploaded a new docker image, updated PostgresTPCDS, and regenerated the q.out files. I'll let the CI run.

[thomasrebele]

There are two test failures (though the CI run seems to be incomplete, as it is marked "aborted"):

• TestLlapZookeeperRegistryImpl had a StackOverflowError. The test passes locally, and I don't think it uses the TPC-DS metastore dump, so it seems to be a flaky failure.
• TestHiveRelJsonSchemaReader checks the schema of TPCDS query 1, and fails because CHAR(16) c_customer_id turned into a VARCHAR(2147483647) c_customer_id. I'll need to do some debugging to understand the cause of this change.

TestHiveRelJsonSchemaReader stack trace

org.opentest4j.AssertionFailedError: expected: <RecordType(BIGINT c_customer_sk, CHAR(16) c_customer_id, BIGINT c_current_cdemo_sk, BIGINT c_current_hdemo_sk, BIGINT c_current_addr_sk, BIGINT c_first_shipto_date_sk, BIGINT c_first_sales_date_sk, CHAR(10) c_salutation, CHAR(20) c_first_name, CHAR(30) c_last_name, CHAR(1) c_preferred_cust_flag, INTEGER c_birth_day, INTEGER c_birth_month, INTEGER c_birth_year, VARCHAR(20) c_birth_country, CHAR(13) c_login, CHAR(50) c_email_address, BIGINT c_last_review_date_sk, BIGINT BLOCK__OFFSET__INSIDE__FILE, VARCHAR(2147483647) INPUT__FILE__NAME, RecordType(BIGINT writeid, INTEGER bucketid, BIGINT rowid) ROW__ID, BOOLEAN ROW__IS__DELETED)> but was: <RecordType(BIGINT c_customer_sk, VARCHAR(2147483647) c_customer_id, BIGINT c_current_cdemo_sk, BIGINT c_current_hdemo_sk, BIGINT c_current_addr_sk, BIGINT c_first_shipto_date_sk, BIGINT c_first_sales_date_sk, CHAR(10) c_salutation, CHAR(20) c_first_name, CHAR(30) c_last_name, CHAR(1) c_preferred_cust_flag, INTEGER c_birth_day, INTEGER c_birth_month, INTEGER c_birth_year, VARCHAR(20) c_birth_country, CHAR(13) c_login, CHAR(50) c_email_address, BIGINT c_last_review_date_sk, BIGINT BLOCK__OFFSET__INSIDE__FILE, VARCHAR(2147483647) INPUT__FILE__NAME, RecordType(BIGINT writeid, INTEGER bucketid, BIGINT rowid) ROW__ID, BOOLEAN ROW__IS__DELETED)>
	at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
	at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
	at org.junit.jupiter.api.AssertEquals.failNotEqual(AssertEquals.java:197)
	at org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:182)
	at org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:177)
	at org.junit.jupiter.api.Assertions.assertEquals(Assertions.java:1145)
	at org.apache.hadoop.hive.ql.optimizer.calcite.TestHiveRelJsonSchemaReader.testReadSchemaFromTpcdsQuery1(TestHiveRelJsonSchemaReader.java:55)

[thomasrebele]

According to the TPC-DS specification (Table 2-14 on page 34), c_customer_id should have the type CHAR(16). It seems that the script to create the table for the new metastore dump used the type string instead:

> select DISTINCT t."TBL_NAME" AS table_name, c."COLUMN_NAME", c."TYPE_NAME" 
  FROM "DBS" d, "TBLS" t, "SDS" s, "COLUMNS_V2" c
  WHERE d."DB_ID" = t."DB_ID" and t."SD_ID" = s."SD_ID" and s."CD_ID" = c."CD_ID"
  and c."COLUMN_NAME" = 'c_customer_id';

 table_name |  COLUMN_NAME  | TYPE_NAME 
------------+---------------+-----------
 customer   | c_customer_id | string
(1 row)

This is the same for column ca_address_id, which also uses string instead of CHAR(16).

@zabetak, would it be ok to fix it in the PostgresTPCDS#install method, or is it required to update the database dump and container image?

[zabetak]

Regarding the type discrepancy on c_customer_id and ca_address_id it may be fine to just update the test code if it affects just a single test.

If fixing the type accuracy can lead to less .q.out discrepancies then I guess its worth doing if its not complicated/risky (we don't want to end-up with a semi corrupted dump with internal inconsistencies).

[thomasrebele]

I've stopped the CI to reduce infrastructure costs. I've updated the image docker.io/thomasrebele/postgres-tpcds-metastore:1.5 with the definition in 9c16680. If you're ok with the changes, @zabetak, I'm happy to run the CI (maybe after updating the docker image to the final URL?).

[zabetak]

I build the image locally on commit 9c16680e8193d55b09ca87b5b1ceafe1e390029f and pushed it to https://hub.docker.com/layers/apache/hive-postgres-tpcds-metastore/1.5 so you can update the PR and run the CI with that one.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[thomasrebele]

The CI got confused about the git history (fatal: refusing to merge unrelated histories). I suggest to rebase the PR before doing a final CI run.

@zabetak, I've removed the custom postgresql.conf. Would you update the docker.io/apache/hive-postgres-tpcds-metastore:1.5 image, please?