Gergely Fürnstáhl has posted comments on this change. ( http://gerrit.cloudera.org:8080/18224 )
Change subject: IMPALA-10948: Default scale and DecimalType ...................................................................... Patch Set 4: (6 comments) http://gerrit.cloudera.org:8080/#/c/18224/4/be/src/exec/parquet/parquet-data-converter.h File be/src/exec/parquet/parquet-data-converter.h: http://gerrit.cloudera.org:8080/#/c/18224/4/be/src/exec/parquet/parquet-data-converter.h@74 PS4, Line 74: if (parquet_element_->__isset.logicalType : && parquet_element_->logicalType.__isset.DECIMAL) : return parquet_element_->logicalType.DECIMAL.precision; > nit: multi-line if stmts should use braces. Done http://gerrit.cloudera.org:8080/#/c/18224/4/be/src/exec/parquet/parquet-metadata-utils.cc File be/src/exec/parquet/parquet-metadata-utils.cc: http://gerrit.cloudera.org:8080/#/c/18224/4/be/src/exec/parquet/parquet-metadata-utils.cc@208 PS4, Line 208: Precision is required, this should be called after checking IsPrecisionSet > We could add a DCHECK(IsPrecisionSet(schema_element)); Done http://gerrit.cloudera.org:8080/#/c/18224/4/testdata/data/README File testdata/data/README: http://gerrit.cloudera.org:8080/#/c/18224/4/testdata/data/README@682 PS4, Line 682: .__set_scale(1); > Do we need this line? With scale=1 I wanted to showcase it does not read out the default 0 from there, rather getting it from the getter. http://gerrit.cloudera.org:8080/#/c/18224/4/testdata/data/README@684 PS4, Line 684: + file_metadata_.schema[1].logicalType.DECIMAL.scale = 1; : + file_metadata_.schema[1].logicalType.__isset.DECIMAL = false; > Are these lines needed? With scale=1 I wanted to showcase it does not read out the default 0 from there, rather getting it from the getter. Possibly__isset.logicalType =false is enough, but I think the intention is more clear this way http://gerrit.cloudera.org:8080/#/c/18224/4/tests/query_test/test_scanners.py File tests/query_test/test_scanners.py: http://gerrit.cloudera.org:8080/#/c/18224/4/tests/query_test/test_scanners.py@393 PS4, Line 393: > nit: unnecessary blank line Done http://gerrit.cloudera.org:8080/#/c/18224/4/tests/query_test/test_scanners.py@394 PS4, Line 394: default-scale > default-scale.test is not added to the PS. Done -- To view, visit http://gerrit.cloudera.org:8080/18224 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I003220b6e2ef39d25d1c33df62c8432803fdc6eb Gerrit-Change-Number: 18224 Gerrit-PatchSet: 4 Gerrit-Owner: Gergely Fürnstáhl <gfurnst...@cloudera.com> Gerrit-Reviewer: Gergely Fürnstáhl <gfurnst...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Thu, 17 Feb 2022 12:08:28 +0000 Gerrit-HasComments: Yes
