Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/22293 )
Change subject: IMPALA-13627: Handle legacy Hive timezone conversion ...................................................................... Patch Set 12: (7 comments) http://gerrit.cloudera.org:8080/#/c/22293/11/be/src/benchmarks/convert-timestamp-benchmark.cc File be/src/benchmarks/convert-timestamp-benchmark.cc: http://gerrit.cloudera.org:8080/#/c/22293/11/be/src/benchmarks/convert-timestamp-benchmark.cc@115 PS11, Line 115: FromUnixTimeNanos: Function iters/ms 10%ile 50%ile 90%ile 10%ile 50%ile 90%ile : (relative) (relative) (relative) : --------------------------------------------------------------------------------------------------------- : (sec split (old)) 31.2 32 32.4 1X 1X 1X : (sec split (new)) 16.3 16.5 16.6 0.523X 0.517X 0.513X : : FromSubsecondUnixTime: Function iters/ms 10%ile 50%ile 90%ile 10%ile 50%ile 90%ile : (relative) (relative) (relative) : --------------------------------------------------------------------------------------------------------- : (old) 32 32.5 33 1X 1X 1X : (new) 14.7 14.9 15.1 0.46X 0.458X 0.458X > Why the number looks regress here? Relative of (new) < Relative of (old). D That's a really good question. Most other numbers improved. I'll look into it. FromUnixTimeNanos (new) is also slower than it was before. http://gerrit.cloudera.org:8080/#/c/22293/11/common/thrift/ImpalaService.thrift File common/thrift/ImpalaService.thrift: http://gerrit.cloudera.org:8080/#/c/22293/11/common/thrift/ImpalaService.thrift@981 PS11, Line 981: CONVERT_LEGACY_HIVE_PARQUET_UTC_TIMESTAMPS > nit: Please uppercase to highlight that this is an Impala query option. Done http://gerrit.cloudera.org:8080/#/c/22293/11/docs/topics/impala_timestamp.xml File docs/topics/impala_timestamp.xml: http://gerrit.cloudera.org:8080/#/c/22293/11/docs/topics/impala_timestamp.xml@292 PS11, Line 292: hive.parquet.timestamp.legacy.conversion.enable > hive.parquet.timestamp.legacy.conversion.enabled Done http://gerrit.cloudera.org:8080/#/c/22293/11/tests/query_test/test_hive_timestamp_conversion.py File tests/query_test/test_hive_timestamp_conversion.py: http://gerrit.cloudera.org:8080/#/c/22293/11/tests/query_test/test_hive_timestamp_conversion.py@18 PS11, Line 18: from __future__ import absolute_import, division, print_fu > nit: This is better as code comment for TestHiveParquetTimestampConversion I think this was accidentally copied from another file. I'll provide a more relevant comment. http://gerrit.cloudera.org:8080/#/c/22293/11/tests/query_test/test_hive_timestamp_conversion.py@32 PS11, Line 32: : @classmethod > Individual .test file is short. Therefore, tests in this class seems better This contradicts Csaba's review. Each test case result is unique. I don't see much value in testing all parameter combinations for each test. http://gerrit.cloudera.org:8080/#/c/22293/11/tests/query_test/test_hive_timestamp_conversion.py@55 PS11, Line 55: > What is the value of this metadata in employee_hive_3_1_3_us_pacific.parque See the commit message for a description of different file metadata scenarios. http://gerrit.cloudera.org:8080/#/c/22293/11/tests/query_test/test_hive_timestamp_conversion.py@67 PS11, Line 67: : def test_hive_3_mixed(self, vector, unique_database): : """Test table containing Hive legacy timestamps written with Hive prior to 3.1.3. : > Can you mention something about timezone="Asia/Singapore" inside timestamp- Done -- To view, visit http://gerrit.cloudera.org:8080/22293 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1271ed1da0b74366ab8315e7ec2d4ee47111e067 Gerrit-Change-Number: 22293 Gerrit-PatchSet: 12 Gerrit-Owner: Michael Smith <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Comment-Date: Tue, 11 Feb 2025 23:24:54 +0000 Gerrit-HasComments: Yes
