Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/21142 )
Change subject: IMPALA-12737: List columns in profile and query history ...................................................................... Patch Set 27: (4 comments) http://gerrit.cloudera.org:8080/#/c/21142/27//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21142/27//COMMIT_MSG@9 PS27, Line 9: Adds "Select Columns", "Where Columns", "Join Columns", "Aggregate : Columns", and "OrderBy Columns" > I think this patch has grown beyond just adding new columns in WM table. Agreed. http://gerrit.cloudera.org:8080/#/c/21142/27/be/src/kudu/util/version_util.h File be/src/kudu/util/version_util.h: http://gerrit.cloudera.org:8080/#/c/21142/27/be/src/kudu/util/version_util.h@42 PS27, Line 42: // Compare two Version objects. Versions that contain an extra component sort before : // (less than) versions that do not contain an extra component. No special handling is : // done of the extra component. Thus '-SNAPSHOT' sorts as greater than '-RELEASE'. : // Example sort order (from least to greatest): : // 1.0.0-RELEASE, 1.0.0-SNAPSHOT, 1.0.0, 1.0.1-SNAPSHOT. : bool operator<(const Version& other) const; : bool operator<=(const Version& other) const; : bool operator>(const Version& other) const; > In general, I'm worried about making changes under be/src/kudu/* since we u All these changes can be done external to this file. Comparison operators can be inside or outside the class, see https://en.cppreference.com/w/cpp/language/operator_comparison. The non-default constructor can be added as a helper function instead. http://gerrit.cloudera.org:8080/#/c/21142/27/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/21142/27/be/src/service/impala-server.cc@760 PS27, Line 760: MonotonicNanos > Use MonotonicMillis() instead? This was part of a rebase. http://gerrit.cloudera.org:8080/#/c/21142/27/be/src/service/workload-management-fields.cc File be/src/service/workload-management-fields.cc: http://gerrit.cloudera.org:8080/#/c/21142/27/be/src/service/workload-management-fields.cc@454 PS27, Line 454: }, VERSION_2_0_0), nit: major version usually signifies a breaking change, would this make more sense as 1.1.0? -- To view, visit http://gerrit.cloudera.org:8080/21142 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I78f3670b067c0c192ee8a212fba95466fbcb51d7 Gerrit-Change-Number: 21142 Gerrit-PatchSet: 27 Gerrit-Owner: Michael Smith <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Comment-Date: Wed, 31 Jul 2024 20:55:02 +0000 Gerrit-HasComments: Yes
