Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/18990 )
Change subject: IMPALA-10660: Impala shell prints DOUBLEs with less precision in HS2 than beeswax ...................................................................... Patch Set 5: (9 comments) http://gerrit.cloudera.org:8080/#/c/18990/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18990/5//COMMIT_MSG@9 PS5, Line 9: fp_format_specification The option name was changed http://gerrit.cloudera.org:8080/#/c/18990/5//COMMIT_MSG@10 PS5, Line 10: which manipulates the print format of floating-point values. "in hs2?" http://gerrit.cloudera.org:8080/#/c/18990/5//COMMIT_MSG@16 PS5, Line 16: This option does not support the beeswax protocol. can you add the reason? (converting to string on server side) http://gerrit.cloudera.org:8080/#/c/18990/5/shell/impala_shell.py File shell/impala_shell.py: http://gerrit.cloudera.org:8080/#/c/18990/5/shell/impala_shell.py@1932 PS5, Line 1932: "not supported with Beeswax protocol") nit: +4 indentation http://gerrit.cloudera.org:8080/#/c/18990/5/shell/impala_shell.py@2055 PS5, Line 2055: options.hs2_fp_format, file=sys.stderr) nit: +4 indentation http://gerrit.cloudera.org:8080/#/c/18990/5/shell/impala_shell.py@2059 PS5, Line 2059: file=sys.stderr) nit: +4 indentation http://gerrit.cloudera.org:8080/#/c/18990/5/shell/value_converter.py File shell/value_converter.py: http://gerrit.cloudera.org:8080/#/c/18990/5/shell/value_converter.py@24 PS5, Line 24: pass : : def override_floating_point_converter(format_specification): : pass : : : class HS2ValueConverter(ValueConverter): : : def __init__(self): : self.value_converters = { : TTypeId.BOOLEAN_TYPE: lambda b: 'true' if b else 'false', : TTypeId.TINYINT_TYPE: str, : TTypeId.SMALLINT_TYPE: str, : TTypeId.INT_TYPE: str, : TTypeId.BIGINT_TYPE: str, : TTypeId.BINARY_TYPE: str, : TTypeId.FLOAT_TYPE: str, : TTypeId.DOUBLE_TYPE: str : } : : def get_converter(self, value): : return self.value_converters.get(value, None) : : def override_floating_point_converter(self, format_specification): : def convert(value): : return ('{:%s}' % format_specification).format(value) : self.value_converters[TTypeId.FLOAT_TYPE] = convert : self.value_converters[TTypeId.DOUBLE_TYPE] = convert nit: for starting of new blocks we generally use indentation of 2 http://gerrit.cloudera.org:8080/#/c/18990/5/tests/shell/test_shell_commandline.py File tests/shell/test_shell_commandline.py: http://gerrit.cloudera.org:8080/#/c/18990/5/tests/shell/test_shell_commandline.py@1326 PS5, Line 1326: test_fp_format_float optional: I think that that most tests could be unified to one, e.g. test_hs2_fp_format. This is contrary to the theory of "one test should only test one thing", but could make the tests more readable http://gerrit.cloudera.org:8080/#/c/18990/5/tests/shell/test_shell_commandline.py@1344 PS5, Line 1344: value = '123456789123456789' : expected = '+123456789123456784.000000' : args = ['--hs2_fp_format', '+f', '-q', : 'select cast("%s" as double) as fp_value' % value] : result = run_impala_shell_cmd(vector, args) : : assert expected in result.stdout optional: This pattern is repeated in most test - there could be function like validate_fp_format(sql_type, fp_format, value, expected) that runs impala shell with the given parameters and checks the results -- To view, visit http://gerrit.cloudera.org:8080/18990 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I424339266be66437941be8bafaa83fa0f2dfbd4e Gerrit-Change-Number: 18990 Gerrit-PatchSet: 5 Gerrit-Owner: Peter Rozsa <pro...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Daniel Becker <daniel.bec...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Peter Rozsa <pro...@cloudera.com> Gerrit-Comment-Date: Thu, 22 Sep 2022 11:43:56 +0000 Gerrit-HasComments: Yes
