Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/12498 )
Change subject: IMPALA-8108: Impala query returns TIMESTAMP values in different types ...................................................................... Patch Set 3: (3 comments) Some high level concerns: this seems like a potentially breaking change, as some workloads may depend on the current way of printing timestamps without subseconds. Adding a flag to change this behavior would be a relatively easy way to avoid breaking things by default. TimestampValue's ToString() function is used in a lot of places internally. If the goal is to return the results differently to the user, then it is enough to change the way Impala returns timestamps in HS2/Beeswax. Or do you also want to change the way cast(timestamp_column as string) works? I guess that not every existing test was run, as I would expect a lot of them to fail because of this change. I started a gerrit-verify-dryrun build on jenkins to verify this. http://gerrit.cloudera.org:8080/#/c/12498/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12498/3//COMMIT_MSG@10 PS3, Line 10: : seconds if they are all zeros. nit: long line, commit message lines are capped at 72 characters http://gerrit.cloudera.org:8080/#/c/12498/3/be/src/runtime/timestamp-value.cc File be/src/runtime/timestamp-value.cc: http://gerrit.cloudera.org:8080/#/c/12498/3/be/src/runtime/timestamp-value.cc@211 PS3, Line 211: BOOST_DATE_TIME_POSIX_TIME_STD_CONFIG What is the role of this config? Generally we assume that Impala works with nanosec precision and lot of tests would break in a different build. If this will be always defined and the #else path is not tested, then I would prefer a compile error if it is not defined. e.g. #ifndef BOOST_DATE_TIME_POSIX_TIME_STD_CONFIG #error "BOOST_DATE_TIME_POSIX_TIME_STD_CONFIG should be defined." #endif http://gerrit.cloudera.org:8080/#/c/12498/3/testdata/workloads/functional-query/queries/QueryTest/timestamps.test File testdata/workloads/functional-query/queries/QueryTest/timestamps.test: http://gerrit.cloudera.org:8080/#/c/12498/3/testdata/workloads/functional-query/queries/QueryTest/timestamps.test@3 PS3, Line 3: nit: trailing spaces -- To view, visit http://gerrit.cloudera.org:8080/12498 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iad9a4659ff6d5353329e9d062886ee80296b5b43 Gerrit-Change-Number: 12498 Gerrit-PatchSet: 3 Gerrit-Owner: Robbie Zhang <rzh...@cloudera.com> Gerrit-Reviewer: Attila Jeges <atti...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Greg Rahn <gr...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Comment-Date: Fri, 15 Feb 2019 17:44:03 +0000 Gerrit-HasComments: Yes
