You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Volodymyr Verovkin (Code Review)" <ge...@cloudera.org> on 2019/10/14 17:16:48 UTC
[kudu-CR] Add a DATE type backed by INT32 (Part 1, C++ client)
Volodymyr Verovkin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/14427
Change subject: Add a DATE type backed by INT32 (Part 1, C++ client)
......................................................................
Add a DATE type backed by INT32 (Part 1, C++ client)
This adds a new DATE type, represented by an INT32 and
that should store the number of days from the Unix epoch, January 1,
1970.
Change-Id: I1d803b6eb573a0b36c99c5a2012f12319a548986
---
M src/kudu/client/client-unittest.cc
M src/kudu/client/predicate-test.cc
M src/kudu/client/scan_batch.cc
M src/kudu/client/scan_batch.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/value.cc
M src/kudu/common/common.proto
M src/kudu/common/key_util.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/partition.cc
M src/kudu/common/row.h
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/hms/hms_catalog-test.cc
M src/kudu/hms/hms_catalog.cc
M src/kudu/integration-tests/all_types-itest.cc
M src/kudu/integration-tests/hms_itest-base.cc
M src/kudu/tools/data_gen_util.cc
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/tool_action_perf.cc
M src/kudu/tools/tool_action_table.cc
23 files changed, 156 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/14427/1
--
To view, visit http://gerrit.cloudera.org:8080/14427
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I1d803b6eb573a0b36c99c5a2012f12319a548986
Gerrit-Change-Number: 14427
Gerrit-PatchSet: 1
Gerrit-Owner: Volodymyr Verovkin <ve...@cloudera.com>
[kudu-CR] [KUDU-2632] Add a DATE type backed by INT32 (Part 1, C++ client)
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/14427 )
Change subject: [KUDU-2632] Add a DATE type backed by INT32 (Part 1, C++ client)
......................................................................
Patch Set 9:
It looks like this has some IWYU failures
--
To view, visit http://gerrit.cloudera.org:8080/14427
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1d803b6eb573a0b36c99c5a2012f12319a548986
Gerrit-Change-Number: 14427
Gerrit-PatchSet: 9
Gerrit-Owner: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Mon, 09 Dec 2019 21:29:47 +0000
Gerrit-HasComments: No
[kudu-CR] Add a DATE type backed by INT32 (Part 1, C++ client)
Posted by "Volodymyr Verovkin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, Greg Solovyev,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14427
to look at the new patch set (#2).
Change subject: Add a DATE type backed by INT32 (Part 1, C++ client)
......................................................................
Add a DATE type backed by INT32 (Part 1, C++ client)
This adds a new DATE type, represented by an INT32 and
that should store the number of days from the Unix epoch, January 1,
1970.
Change-Id: I1d803b6eb573a0b36c99c5a2012f12319a548986
---
M src/kudu/client/client-unittest.cc
M src/kudu/client/predicate-test.cc
M src/kudu/client/scan_batch.cc
M src/kudu/client/scan_batch.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/value.cc
M src/kudu/common/common.proto
M src/kudu/common/key_util.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/partition.cc
M src/kudu/common/row.h
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/hms/hms_catalog-test.cc
M src/kudu/hms/hms_catalog.cc
M src/kudu/integration-tests/all_types-itest.cc
M src/kudu/integration-tests/hms_itest-base.cc
M src/kudu/tools/data_gen_util.cc
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/tool_action_perf.cc
M src/kudu/tools/tool_action_table.cc
23 files changed, 156 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/14427/2
--
To view, visit http://gerrit.cloudera.org:8080/14427
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1d803b6eb573a0b36c99c5a2012f12319a548986
Gerrit-Change-Number: 14427
Gerrit-PatchSet: 2
Gerrit-Owner: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [KUDU-2632] Add a DATE type backed by INT32 (Part 1, C++ client)
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14427 )
Change subject: [KUDU-2632] Add a DATE type backed by INT32 (Part 1, C++ client)
......................................................................
Patch Set 5: Code-Review+1
(1 comment)
When Attila did the VARCHAR/CHAR work, he prototyped the Impala side changes before merging the Kudu-side changes. This turned out to be a very good idea given how much difficulty he had with CHAR in Impala.
So while I think this is ready to be merged, I'm gonna give a +1 and not a +2 until you've had a chance to modify Impala to use this and test that it works. You'll probably need the Java client change for that to work as Impala uses both the C++ and Java clients.
Changes for the other integrations (Python, Spark, Hive, Nifi, etc.) are simpler and can be handled after merging the core stuff.
http://gerrit.cloudera.org:8080/#/c/14427/5/src/kudu/common/types.cc
File src/kudu/common/types.cc:
http://gerrit.cloudera.org:8080/#/c/14427/5/src/kudu/common/types.cc@117
PS5, Line 117: constexpr static const char* kDateFormat = "%F"; // the ISO 8601 date format
: static constexpr time_t kSecondsInDay = 24 * 60 * 60;
adar@adar-Precision-5520:~/Source/kudu$ git grep "static constexpr" | wc -l
120
adar@adar-Precision-5520:~/Source/kudu$ git grep "constexpr static" | wc -l
4
adar@adar-Precision-5520:~/Source/kudu$ git grep "static const" | wc -l
917
adar@adar-Precision-5520:~/Source/kudu$ git grep "const static" | wc -l
0
So kDateFormat should be "static constexpr ..."
--
To view, visit http://gerrit.cloudera.org:8080/14427
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1d803b6eb573a0b36c99c5a2012f12319a548986
Gerrit-Change-Number: 14427
Gerrit-PatchSet: 5
Gerrit-Owner: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 17 Oct 2019 04:06:18 +0000
Gerrit-HasComments: Yes
[kudu-CR] [KUDU-2632] Add a DATE type backed by INT32 (Part 1, C++ client)
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14427 )
Change subject: [KUDU-2632] Add a DATE type backed by INT32 (Part 1, C++ client)
......................................................................
Patch Set 12: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/14427
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1d803b6eb573a0b36c99c5a2012f12319a548986
Gerrit-Change-Number: 14427
Gerrit-PatchSet: 12
Gerrit-Owner: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 17 Dec 2019 19:57:22 +0000
Gerrit-HasComments: No
[kudu-CR] [KUDU-2632] Add a DATE type backed by INT32 (Part 1, C++ client)
Posted by "Volodymyr Verovkin (Code Review)" <ge...@cloudera.org>.
Volodymyr Verovkin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14427 )
Change subject: [KUDU-2632] Add a DATE type backed by INT32 (Part 1, C++ client)
......................................................................
Patch Set 2:
(10 comments)
http://gerrit.cloudera.org:8080/#/c/14427/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14427/2//COMMIT_MSG@7
PS2, Line 7: Add a DATE type backed by INT32 (Part 1, C++ client)
> Tag with KUDU-2632.
Done
http://gerrit.cloudera.org:8080/#/c/14427/2//COMMIT_MSG@9
PS2, Line 9: This adds a new DATE type, represented by an INT32 and
> Could you add some commentary about validation (or lack thereof)? For examp
Done
http://gerrit.cloudera.org:8080/#/c/14427/2/src/kudu/common/partial_row.h
File src/kudu/common/partial_row.h:
http://gerrit.cloudera.org:8080/#/c/14427/2/src/kudu/common/partial_row.h@141
PS2, Line 141: Status SetUnixTimeMicros(int col_idx, int64_t micros_since_utc_epoch) WARN_UNUSED_RESULT;
> warning: function 'kudu::KuduPartialRow::SetUnixTimeMicros' has a definitio
Done
http://gerrit.cloudera.org:8080/#/c/14427/2/src/kudu/common/partial_row.h@142
PS2, Line 142: Status SetDate(int col_idx, int32_t days_since_unix_epoch) WARN_UNUSED_RESULT;
> warning: function 'kudu::KuduPartialRow::SetDate' has a definition with dif
Done
http://gerrit.cloudera.org:8080/#/c/14427/2/src/kudu/common/partial_row.h@365
PS2, Line 365: Status GetUnixTimeMicros(const Slice& col_name,
: int64_t* micros_since_utc_epoch) const WARN_UNUSED_RESULT;
: Status GetDate(const Slice& col_name,
: int32_t* days_since_unix_epoch) const WARN_UNUSED_RESULT;
> Nit: could you fix the indentation here?
Done
http://gerrit.cloudera.org:8080/#/c/14427/2/src/kudu/common/row.h
File src/kudu/common/row.h:
http://gerrit.cloudera.org:8080/#/c/14427/2/src/kudu/common/row.h@669
PS2, Line 669: void AddDate(int32_t seconds_utc_since_epoch) {
> Should be days_since, right?
Done
http://gerrit.cloudera.org:8080/#/c/14427/2/src/kudu/common/types.h
File src/kudu/common/types.h:
http://gerrit.cloudera.org:8080/#/c/14427/2/src/kudu/common/types.h@582
PS2, Line 582: struct DataTypeTraits<DATE> : public DerivedTypeTraits<INT32>{
> Does it make sense to add a unit test for DataTypeTraits<DATA>::AppendDebug
Done
http://gerrit.cloudera.org:8080/#/c/14427/2/src/kudu/common/types.h@583
PS2, Line 583: %Y-%m-%d
> Maybe, simply use %F and add a comment that this is ISO 8601 date format?
Done
http://gerrit.cloudera.org:8080/#/c/14427/2/src/kudu/common/types.h@593
PS2, Line 593: gmtime_r(&days_since_unix_epoch, &tm_info);
> I assume this does the right thing when days_since_unix_epoch is negative?
Yes, I checked.
gmtime_r produces correct output for all int64_t values.
http://gerrit.cloudera.org:8080/#/c/14427/2/src/kudu/common/types.h@595
PS2, Line 595: strftime
> Though highly unlikely to get some unexpected output here, but does it make
Done
--
To view, visit http://gerrit.cloudera.org:8080/14427
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1d803b6eb573a0b36c99c5a2012f12319a548986
Gerrit-Change-Number: 14427
Gerrit-PatchSet: 2
Gerrit-Owner: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 16 Oct 2019 08:39:40 +0000
Gerrit-HasComments: Yes
[kudu-CR] [KUDU-2632] Add a DATE type backed by INT32 (Part 1, C++ client)
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14427 )
Change subject: [KUDU-2632] Add a DATE type backed by INT32 (Part 1, C++ client)
......................................................................
Patch Set 6: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14427/5/src/kudu/common/types.cc
File src/kudu/common/types.cc:
http://gerrit.cloudera.org:8080/#/c/14427/5/src/kudu/common/types.cc@117
PS5, Line 117: void DataTypeTraits<DATE>::AppendDebugStringForValue(const void* val, string* str) {
: constexpr static const char* kDateFormat = "%F"; // t
> adar@adar-Precision-5520:~/Source/kudu$ git grep "static constexpr" | wc -l
Looks like you missed this one.
--
To view, visit http://gerrit.cloudera.org:8080/14427
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1d803b6eb573a0b36c99c5a2012f12319a548986
Gerrit-Change-Number: 14427
Gerrit-PatchSet: 6
Gerrit-Owner: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Fri, 25 Oct 2019 03:08:28 +0000
Gerrit-HasComments: Yes
[kudu-CR] [KUDU-2632] Add a DATE type backed by INT32 (Part 1, C++ client)
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14427 )
Change subject: [KUDU-2632] Add a DATE type backed by INT32 (Part 1, C++ client)
......................................................................
Patch Set 11:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14427/7/src/kudu/integration-tests/all_types-itest.cc
File src/kudu/integration-tests/all_types-itest.cc:
http://gerrit.cloudera.org:8080/#/c/14427/7/src/kudu/integration-tests/all_types-itest.cc@578
PS7, Line 578: IntKeysTestSetup<KeyTypeWrapper<UNIXTIME_MICROS> >,
This seems like pretty important test coverage for data types that can be part of a PK. Is there a fundamental reason why DATE can't participate? If not, could you write an appropriate "setup" struct?
--
To view, visit http://gerrit.cloudera.org:8080/14427
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1d803b6eb573a0b36c99c5a2012f12319a548986
Gerrit-Change-Number: 14427
Gerrit-PatchSet: 11
Gerrit-Owner: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 10 Dec 2019 05:07:38 +0000
Gerrit-HasComments: Yes
[kudu-CR] [KUDU-2632] Add a DATE type backed by INT32 (Part 1, C++ client)
Posted by "Volodymyr Verovkin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, Greg Solovyev, Grant Henke,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14427
to look at the new patch set (#11).
Change subject: [KUDU-2632] Add a DATE type backed by INT32 (Part 1, C++ client)
......................................................................
[KUDU-2632] Add a DATE type backed by INT32 (Part 1, C++ client)
This adds a new DATE type, represented by an INT32 and
that should store the number of days from the Unix epoch, January 1,
1970.
Range:
0001-01-01 to 9999-12-31
or
-719162 to 2932896 (days; computed with mktime()/(24*60*60),
timezone = GMT, daylight saving = 0)
Range validation is done in KuduPartialRow::SetDate() function.
Change-Id: I1d803b6eb573a0b36c99c5a2012f12319a548986
---
M src/kudu/client/client-unittest.cc
M src/kudu/client/predicate-test.cc
M src/kudu/client/scan_batch.cc
M src/kudu/client/scan_batch.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/value.cc
M src/kudu/common/common.proto
M src/kudu/common/key_util.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/partition.cc
M src/kudu/common/row.h
M src/kudu/common/types-test.cc
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/hms/hms_catalog-test.cc
M src/kudu/hms/hms_catalog.cc
M src/kudu/integration-tests/all_types-itest.cc
M src/kudu/integration-tests/data_gen_util.cc
M src/kudu/integration-tests/hms_itest-base.cc
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/tool_action_perf.cc
M src/kudu/tools/tool_action_table.cc
24 files changed, 235 insertions(+), 24 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/14427/11
--
To view, visit http://gerrit.cloudera.org:8080/14427
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1d803b6eb573a0b36c99c5a2012f12319a548986
Gerrit-Change-Number: 14427
Gerrit-PatchSet: 11
Gerrit-Owner: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
[kudu-CR] [KUDU-2632] Add a DATE type backed by INT32 (Part 1, C++ client)
Posted by "Volodymyr Verovkin (Code Review)" <ge...@cloudera.org>.
Volodymyr Verovkin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14427 )
Change subject: [KUDU-2632] Add a DATE type backed by INT32 (Part 1, C++ client)
......................................................................
Patch Set 12:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/14427/11/src/kudu/common/partial_row.h
File src/kudu/common/partial_row.h:
http://gerrit.cloudera.org:8080/#/c/14427/11/src/kudu/common/partial_row.h@450
PS11, Line 450:
> style nit: if continuing a statement on the next line, shift is two times o
Done
http://gerrit.cloudera.org:8080/#/c/14427/11/src/kudu/common/partial_row.cc
File src/kudu/common/partial_row.cc:
http://gerrit.cloudera.org:8080/#/c/14427/11/src/kudu/common/partial_row.cc@50
PS11, Line 50: const Schema& schema
> style nit: I'm not sure we widely use constant pointers as input parameters
Done
http://gerrit.cloudera.org:8080/#/c/14427/11/src/kudu/common/partial_row.cc@715
PS11, Line 715: int32_t* d
> nit: fix the alignment
Done
http://gerrit.cloudera.org:8080/#/c/14427/11/src/kudu/common/types-test.cc
File src/kudu/common/types-test.cc:
http://gerrit.cloudera.org:8080/#/c/14427/11/src/kudu/common/types-test.cc@56
PS11, Line 56: TestDateToString("1-01-01", *DataTypeTraits<DATE>::min_value());
: TestDateToString("9999-12-31", *DataTypeTraits<DATE>::max_value());
> Does it make sense to add a couple more cases to test for off-by-one issues
Done
http://gerrit.cloudera.org:8080/#/c/14427/11/src/kudu/common/types.h
File src/kudu/common/types.h:
http://gerrit.cloudera.org:8080/#/c/14427/11/src/kudu/common/types.h@604
PS11, Line 604: };
> nit: remove the extra line
Done
http://gerrit.cloudera.org:8080/#/c/14427/7/src/kudu/integration-tests/all_types-itest.cc
File src/kudu/integration-tests/all_types-itest.cc:
http://gerrit.cloudera.org:8080/#/c/14427/7/src/kudu/integration-tests/all_types-itest.cc@578
PS7, Line 578: KuduScanner scanner(table_.get());
> This seems like pretty important test coverage for data types that can be p
Done
--
To view, visit http://gerrit.cloudera.org:8080/14427
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1d803b6eb573a0b36c99c5a2012f12319a548986
Gerrit-Change-Number: 14427
Gerrit-PatchSet: 12
Gerrit-Owner: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 17 Dec 2019 01:36:44 +0000
Gerrit-HasComments: Yes
[kudu-CR] [KUDU-2632] Add a DATE type backed by INT32 (Part 1, C++ client)
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14427 )
Change subject: [KUDU-2632] Add a DATE type backed by INT32 (Part 1, C++ client)
......................................................................
Patch Set 4:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/14427/4/src/kudu/common/partial_row.cc
File src/kudu/common/partial_row.cc:
http://gerrit.cloudera.org:8080/#/c/14427/4/src/kudu/common/partial_row.cc@298
PS4, Line 298: if (!DataTypeTraits<DATE>::isValidValue(val)) {
Could add a PREDICT_FALSE here.
http://gerrit.cloudera.org:8080/#/c/14427/4/src/kudu/common/partial_row.cc@296
PS4, Line 296: namespace {
: Status CheckDateValueInRange(int col_idx, int32_t val, const Schema* schema) {
: if (!DataTypeTraits<DATE>::isValidValue(val)) {
: const ColumnSchema& col = schema->column(col_idx);
: return Status::InvalidArgument(
: Substitute("value $0 out of range for date column '$1'", val, col.name()));
: }
: return Status::OK();
: }
: } // anonymous namespace
Nit: move to the top of the file. That's where we usually stash static functions and anonymous namespaces.
http://gerrit.cloudera.org:8080/#/c/14427/4/src/kudu/common/types-test.cc
File src/kudu/common/types-test.cc:
http://gerrit.cloudera.org:8080/#/c/14427/4/src/kudu/common/types-test.cc@54
PS4, Line 54: TestDateToString("1-01-01", *DataTypeTraits<DATE>::min_value());
What are the expectations around leading 0s? Should this be "0001-01-01"?
http://gerrit.cloudera.org:8080/#/c/14427/4/src/kudu/common/types.h
File src/kudu/common/types.h:
http://gerrit.cloudera.org:8080/#/c/14427/4/src/kudu/common/types.h@583
PS4, Line 583: constexpr static const char* kDateFormat = "%F"; // the ISO 8601 date format
: static constexpr time_t kSecondsInDay = 24 * 60 * 60;
These two could be static variables declared in the body of AppendDebugStringForValue as that's the only place they're used.
http://gerrit.cloudera.org:8080/#/c/14427/4/src/kudu/common/types.h@603
PS4, Line 603: static bool isValidValue(int32_t val) {
Nit: should be IsValidValue
http://gerrit.cloudera.org:8080/#/c/14427/4/src/kudu/common/types.cc
File src/kudu/common/types.cc:
http://gerrit.cloudera.org:8080/#/c/14427/4/src/kudu/common/types.cc@116
PS4, Line 116: std::
Nit: add using and drop.
http://gerrit.cloudera.org:8080/#/c/14427/4/src/kudu/common/types.cc@122
PS4, Line 122: Date
Nit: "DATE"
--
To view, visit http://gerrit.cloudera.org:8080/14427
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1d803b6eb573a0b36c99c5a2012f12319a548986
Gerrit-Change-Number: 14427
Gerrit-PatchSet: 4
Gerrit-Owner: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 17 Oct 2019 00:22:49 +0000
Gerrit-HasComments: Yes
[kudu-CR] [KUDU-2632] Add a DATE type backed by INT32 (Part 1, C++ client)
Posted by "Volodymyr Verovkin (Code Review)" <ge...@cloudera.org>.
Volodymyr Verovkin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14427 )
Change subject: [KUDU-2632] Add a DATE type backed by INT32 (Part 1, C++ client)
......................................................................
Patch Set 2:
(15 comments)
http://gerrit.cloudera.org:8080/#/c/14427/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14427/3//COMMIT_MSG@12
PS3, Line 12:
> Where is this enforced/validated?
Done
http://gerrit.cloudera.org:8080/#/c/14427/3//COMMIT_MSG@13
PS3, Line 13: Change-Id: I1d803b6eb573a0b36c99c5a2012f12319a548986
> See KUDU-2632: the SQL spec says we should start with 0001.
Done
http://gerrit.cloudera.org:8080/#/c/14427/3//COMMIT_MSG@15
PS3, Line 15:
> Too long, wrap.
Done
http://gerrit.cloudera.org:8080/#/c/14427/3/src/kudu/client/scan_batch.h
File src/kudu/client/scan_batch.h:
http://gerrit.cloudera.org:8080/#/c/14427/3/src/kudu/client/scan_batch.h@246
PS3, Line 246: Status GetDate(int col_idx, int32_t* days_since_unix_epoch) const WARN_UNUSED_RESULT;
> nit: use days_since_utc_epoch to match GetUnixTimeMicros. Here and elsewher
UTC (GMT) - Universal Time Coordinated, is used to express timezone offset.
https://en.wikipedia.org/wiki/Coordinated_Universal_Time
So, it would be correct to rename "micros_since_utc_epoch" to "micros_since_unix_epoch" here and elsewhere.
http://gerrit.cloudera.org:8080/#/c/14427/3/src/kudu/client/schema.h
File src/kudu/client/schema.h:
http://gerrit.cloudera.org:8080/#/c/14427/3/src/kudu/client/schema.h@192
PS3, Line 192: TIMESTAMP = UNIXTIME_MICROS, //!< deprecated, use UNIXTIME_MICROS
> Nit: Keep timestamp last
Why ? I would place TIMESTAMP right after UNIXTIME_MICROS.
http://gerrit.cloudera.org:8080/#/c/14427/2/src/kudu/common/partial_row.h
File src/kudu/common/partial_row.h:
http://gerrit.cloudera.org:8080/#/c/14427/2/src/kudu/common/partial_row.h@365
PS2, Line 365: Status GetUnixTimeMicros(const Slice& col_name,
: int64_t* micros_since_utc_epoch) const WARN_UNUSED_RESULT;
: Status GetDate(const Slice& col_name,
: int32_t* days_since_unix_epoch) const WARN_UNUSED_RESULT;
> Not done?
Done
http://gerrit.cloudera.org:8080/#/c/14427/3/src/kudu/common/partial_row.cc
File src/kudu/common/partial_row.cc:
http://gerrit.cloudera.org:8080/#/c/14427/3/src/kudu/common/partial_row.cc@265
PS3, Line 265: return Set<TypeTraits<DATE> >(col_name, val);
> Do we validate the range of values anywhere? Per the ANSI SQL spec, the val
Done
http://gerrit.cloudera.org:8080/#/c/14427/3/src/kudu/common/partition.cc
File src/kudu/common/partition.cc:
http://gerrit.cloudera.org:8080/#/c/14427/3/src/kudu/common/partition.cc@1013
PS3, Line 1013: case DATE:
> I think this should be DATE_MIN (0001-01-01) + 1 given the minimum and ma
Done
http://gerrit.cloudera.org:8080/#/c/14427/3/src/kudu/common/partition.cc@1100
PS3, Line 1100: if (value < INT32_MAX) {
> I think this should be checking DATE_MAX (9999-12-3) given the minimum an
Done
http://gerrit.cloudera.org:8080/#/c/14427/3/src/kudu/common/types-test.cc
File src/kudu/common/types-test.cc:
http://gerrit.cloudera.org:8080/#/c/14427/3/src/kudu/common/types-test.cc@59
PS3, Line 59: ASSERT_EQ("1923-12-01T00:44:36.876544Z", result);
> These values should be invalid.
Done
http://gerrit.cloudera.org:8080/#/c/14427/3/src/kudu/common/types.h
File src/kudu/common/types.h:
http://gerrit.cloudera.org:8080/#/c/14427/3/src/kudu/common/types.h@553
PS3, Line 553: static const int US_TO_S = 1000L * 1000L;
> warning: invalid case style for global constant 'US_TO_S' [readability-iden
Done
http://gerrit.cloudera.org:8080/#/c/14427/3/src/kudu/common/types.h@583
PS3, Line 583: constexpr static const char* kDateFormat = "%Y-%m-%d";
> Can the kDateFormat above be used?
The kDateFormat above contains hours, minutes and seconds, which do are not present in DATE type.
http://gerrit.cloudera.org:8080/#/c/14427/3/src/kudu/common/types.h@584
PS3, Line 584:
> This could be moved up by US_TO_S to keep conversions in the same place.
These conversions are specific for each type. So, it should be kept in namespaces of respective classes.
http://gerrit.cloudera.org:8080/#/c/14427/3/src/kudu/common/types.h@593
PS3, Line 593: gmtime_r(&days_since_unix_epoch, &tm_info);
> Could you simplify with StringAppendStrftime (from gutil/walltime.h)?
Done
http://gerrit.cloudera.org:8080/#/c/14427/3/src/kudu/common/types.h@600
PS3, Line 600: template<>
: struct DataTypeTraits<DECIMAL32> : public DerivedTypeTraits<INT32>{
: static const char* name() {
: return "decimal";
> Rewrite as a ternary:
Done
--
To view, visit http://gerrit.cloudera.org:8080/14427
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1d803b6eb573a0b36c99c5a2012f12319a548986
Gerrit-Change-Number: 14427
Gerrit-PatchSet: 2
Gerrit-Owner: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 17 Oct 2019 00:10:37 +0000
Gerrit-HasComments: Yes
[kudu-CR] [KUDU-2632] Add a DATE type backed by INT32 (Part 1, C++ client)
Posted by "Volodymyr Verovkin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, Greg Solovyev, Grant Henke,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14427
to look at the new patch set (#10).
Change subject: [KUDU-2632] Add a DATE type backed by INT32 (Part 1, C++ client)
......................................................................
[KUDU-2632] Add a DATE type backed by INT32 (Part 1, C++ client)
This adds a new DATE type, represented by an INT32 and
that should store the number of days from the Unix epoch, January 1,
1970.
Range:
0001-01-01 to 9999-12-31
or
-719162 to 2932896 (days; computed with mktime()/(24*60*60),
timezone = GMT, daylight saving = 0)
Range validation is done in KuduPartialRow::SetDate() function.
Change-Id: I1d803b6eb573a0b36c99c5a2012f12319a548986
---
M src/kudu/client/client-unittest.cc
M src/kudu/client/predicate-test.cc
M src/kudu/client/scan_batch.cc
M src/kudu/client/scan_batch.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/value.cc
M src/kudu/common/common.proto
M src/kudu/common/key_util.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/partition.cc
M src/kudu/common/row.h
M src/kudu/common/types-test.cc
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/hms/hms_catalog-test.cc
M src/kudu/hms/hms_catalog.cc
M src/kudu/integration-tests/all_types-itest.cc
M src/kudu/integration-tests/data_gen_util.cc
M src/kudu/integration-tests/hms_itest-base.cc
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/tool_action_perf.cc
M src/kudu/tools/tool_action_table.cc
24 files changed, 235 insertions(+), 24 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/14427/10
--
To view, visit http://gerrit.cloudera.org:8080/14427
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1d803b6eb573a0b36c99c5a2012f12319a548986
Gerrit-Change-Number: 14427
Gerrit-PatchSet: 10
Gerrit-Owner: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
[kudu-CR] [KUDU-2632] Add a DATE type backed by INT32 (Part 1, C++ client)
Posted by "Volodymyr Verovkin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, Greg Solovyev, Grant Henke,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14427
to look at the new patch set (#4).
Change subject: [KUDU-2632] Add a DATE type backed by INT32 (Part 1, C++ client)
......................................................................
[KUDU-2632] Add a DATE type backed by INT32 (Part 1, C++ client)
This adds a new DATE type, represented by an INT32 and
that should store the number of days from the Unix epoch, January 1,
1970.
Range:
0001-01-01 to 9999-12-31
or
-719162 to 2932896 (days; computed with mktime()/(24*60*60),
timezone = GMT, daylight saving = 0)
Range validation is done in KuduPartialRow::SetDate() function.
Change-Id: I1d803b6eb573a0b36c99c5a2012f12319a548986
---
M src/kudu/client/client-unittest.cc
M src/kudu/client/predicate-test.cc
M src/kudu/client/scan_batch.cc
M src/kudu/client/scan_batch.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/value.cc
M src/kudu/common/common.proto
M src/kudu/common/key_util.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/partition.cc
M src/kudu/common/row.h
M src/kudu/common/types-test.cc
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/hms/hms_catalog-test.cc
M src/kudu/hms/hms_catalog.cc
M src/kudu/integration-tests/all_types-itest.cc
M src/kudu/integration-tests/hms_itest-base.cc
M src/kudu/tools/data_gen_util.cc
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/tool_action_perf.cc
M src/kudu/tools/tool_action_table.cc
24 files changed, 222 insertions(+), 15 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/14427/4
--
To view, visit http://gerrit.cloudera.org:8080/14427
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1d803b6eb573a0b36c99c5a2012f12319a548986
Gerrit-Change-Number: 14427
Gerrit-PatchSet: 4
Gerrit-Owner: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
[kudu-CR] [KUDU-2632] Add a DATE type backed by INT32 (Part 1, C++ client)
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14427 )
Change subject: [KUDU-2632] Add a DATE type backed by INT32 (Part 1, C++ client)
......................................................................
Patch Set 7:
The test failures look relevant.
--
To view, visit http://gerrit.cloudera.org:8080/14427
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1d803b6eb573a0b36c99c5a2012f12319a548986
Gerrit-Change-Number: 14427
Gerrit-PatchSet: 7
Gerrit-Owner: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Mon, 18 Nov 2019 05:23:58 +0000
Gerrit-HasComments: No
[kudu-CR] [KUDU-2632] Add a DATE type backed by INT32 (Part 1, C++ client)
Posted by "Volodymyr Verovkin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, Greg Solovyev, Grant Henke,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14427
to look at the new patch set (#9).
Change subject: [KUDU-2632] Add a DATE type backed by INT32 (Part 1, C++ client)
......................................................................
[KUDU-2632] Add a DATE type backed by INT32 (Part 1, C++ client)
This adds a new DATE type, represented by an INT32 and
that should store the number of days from the Unix epoch, January 1,
1970.
Range:
0001-01-01 to 9999-12-31
or
-719162 to 2932896 (days; computed with mktime()/(24*60*60),
timezone = GMT, daylight saving = 0)
Range validation is done in KuduPartialRow::SetDate() function.
Change-Id: I1d803b6eb573a0b36c99c5a2012f12319a548986
---
M src/kudu/client/client-unittest.cc
M src/kudu/client/predicate-test.cc
M src/kudu/client/scan_batch.cc
M src/kudu/client/scan_batch.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/value.cc
M src/kudu/common/common.proto
M src/kudu/common/key_util.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/partition.cc
M src/kudu/common/row.h
M src/kudu/common/types-test.cc
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/hms/hms_catalog-test.cc
M src/kudu/hms/hms_catalog.cc
M src/kudu/integration-tests/all_types-itest.cc
M src/kudu/integration-tests/data_gen_util.cc
M src/kudu/integration-tests/hms_itest-base.cc
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/tool_action_perf.cc
M src/kudu/tools/tool_action_table.cc
24 files changed, 223 insertions(+), 15 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/14427/9
--
To view, visit http://gerrit.cloudera.org:8080/14427
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1d803b6eb573a0b36c99c5a2012f12319a548986
Gerrit-Change-Number: 14427
Gerrit-PatchSet: 9
Gerrit-Owner: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
[kudu-CR] [KUDU-2632] Add a DATE type backed by INT32 (Part 1, C++ client)
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/14427 )
Change subject: [KUDU-2632] Add a DATE type backed by INT32 (Part 1, C++ client)
......................................................................
[KUDU-2632] Add a DATE type backed by INT32 (Part 1, C++ client)
This adds a new DATE type, represented by an INT32 and
that should store the number of days from the Unix epoch, January 1,
1970.
Range:
0001-01-01 to 9999-12-31
or
-719162 to 2932896 (days; computed with mktime()/(24*60*60),
timezone = GMT, daylight saving = 0)
Range validation is done in KuduPartialRow::SetDate() function.
Change-Id: I1d803b6eb573a0b36c99c5a2012f12319a548986
Reviewed-on: http://gerrit.cloudera.org:8080/14427
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Reviewed-by: Alexey Serbin <as...@cloudera.com>
---
M src/kudu/client/client-unittest.cc
M src/kudu/client/predicate-test.cc
M src/kudu/client/scan_batch.cc
M src/kudu/client/scan_batch.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/value.cc
M src/kudu/common/common.proto
M src/kudu/common/key_util.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/partition.cc
M src/kudu/common/row.h
M src/kudu/common/types-test.cc
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/hms/hms_catalog-test.cc
M src/kudu/hms/hms_catalog.cc
M src/kudu/integration-tests/all_types-itest.cc
M src/kudu/integration-tests/data_gen_util.cc
M src/kudu/integration-tests/hms_itest-base.cc
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/tool_action_perf.cc
M src/kudu/tools/tool_action_table.cc
24 files changed, 312 insertions(+), 20 deletions(-)
Approvals:
Kudu Jenkins: Verified
Adar Dembo: Looks good to me, approved
Alexey Serbin: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/14427
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I1d803b6eb573a0b36c99c5a2012f12319a548986
Gerrit-Change-Number: 14427
Gerrit-PatchSet: 13
Gerrit-Owner: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
[kudu-CR] [KUDU-2632] Add a DATE type backed by INT32 (Part 1, C++ client)
Posted by "Volodymyr Verovkin (Code Review)" <ge...@cloudera.org>.
Volodymyr Verovkin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14427 )
Change subject: [KUDU-2632] Add a DATE type backed by INT32 (Part 1, C++ client)
......................................................................
Patch Set 5:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/14427/4/src/kudu/common/partial_row.cc
File src/kudu/common/partial_row.cc:
http://gerrit.cloudera.org:8080/#/c/14427/4/src/kudu/common/partial_row.cc@298
PS4, Line 298: Status KuduPartialRow::SetInt32(int col_idx, int32_t val) {
> Could add a PREDICT_FALSE here.
Done
http://gerrit.cloudera.org:8080/#/c/14427/4/src/kudu/common/partial_row.cc@296
PS4, Line 296: return Set<TypeTraits<INT16> >(col_idx, val);
: }
: Status KuduPartialRow::SetInt32(int col_idx, int32_t val) {
: return Set<TypeTraits<INT32> >(col_idx, val);
: }
: Status KuduPartialRow::SetInt64(int col_idx, int64_t val) {
: return Set<TypeTraits<INT64> >(col_idx, val);
: }
: Status KuduPartialRow::SetUnixTimeMicros(int col_idx, int64_t micros_since_utc_epoch) {
: return Set<TypeTraits<
> Nit: move to the top of the file. That's where we usually stash static func
Done
http://gerrit.cloudera.org:8080/#/c/14427/4/src/kudu/common/types-test.cc
File src/kudu/common/types-test.cc:
http://gerrit.cloudera.org:8080/#/c/14427/4/src/kudu/common/types-test.cc@54
PS4, Line 54: TestDateToString("1-01-01", *DataTypeTraits<DATE>::min_value());
> What are the expectations around leading 0s? Should this be "0001-01-01"?
1) It's just AppendDebugStringForValue() function.
2) There is no built-in format for 4 digit year.
http://gerrit.cloudera.org:8080/#/c/14427/4/src/kudu/common/types.h
File src/kudu/common/types.h:
http://gerrit.cloudera.org:8080/#/c/14427/4/src/kudu/common/types.h@583
PS4, Line 583: static constexpr int32_t kMinValue = -719162; // mktime(0001-01-01)
: static constexpr int32_t kMaxValue = 2932896; // mkti
> These two could be static variables declared in the body of AppendDebugStri
Done
http://gerrit.cloudera.org:8080/#/c/14427/4/src/kudu/common/types.h@603
PS4, Line 603: }
> Nit: should be IsValidValue
Done
http://gerrit.cloudera.org:8080/#/c/14427/4/src/kudu/common/types.cc
File src/kudu/common/types.cc:
http://gerrit.cloudera.org:8080/#/c/14427/4/src/kudu/common/types.cc@116
PS4, Line 116: strin
> Nit: add using and drop.
Done
http://gerrit.cloudera.org:8080/#/c/14427/4/src/kudu/common/types.cc@122
PS4, Line 122: ix_e
> Nit: "DATE"
Done
--
To view, visit http://gerrit.cloudera.org:8080/14427
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1d803b6eb573a0b36c99c5a2012f12319a548986
Gerrit-Change-Number: 14427
Gerrit-PatchSet: 5
Gerrit-Owner: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 17 Oct 2019 01:29:23 +0000
Gerrit-HasComments: Yes
[kudu-CR] Add a DATE type backed by INT32 (Part 1, C++ client)
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14427 )
Change subject: Add a DATE type backed by INT32 (Part 1, C++ client)
......................................................................
Patch Set 2:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/14427/2/src/kudu/common/partial_row.cc
File src/kudu/common/partial_row.cc:
http://gerrit.cloudera.org:8080/#/c/14427/2/src/kudu/common/partial_row.cc@659
PS2, Line 659:
nit: fix the indent
http://gerrit.cloudera.org:8080/#/c/14427/2/src/kudu/common/types.h
File src/kudu/common/types.h:
http://gerrit.cloudera.org:8080/#/c/14427/2/src/kudu/common/types.h@582
PS2, Line 582: struct DataTypeTraits<DATE> : public DerivedTypeTraits<INT32>{
Does it make sense to add a unit test for DataTypeTraits<DATA>::AppendDebugStringForValue() to cover the conversion of date as number of seconds from the start of the Unix epoch into a string? Something like TestTypes.TestTimestampPrinting ?
http://gerrit.cloudera.org:8080/#/c/14427/2/src/kudu/common/types.h@583
PS2, Line 583: %Y-%m-%d
Maybe, simply use %F and add a comment that this is ISO 8601 date format?
http://gerrit.cloudera.org:8080/#/c/14427/2/src/kudu/common/types.h@595
PS2, Line 595: strftime
Though highly unlikely to get some unexpected output here, but does it make sense to check for the return code of the strftime() function, handling the case when 'date' would contain unexpected contents as the result?
--
To view, visit http://gerrit.cloudera.org:8080/14427
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1d803b6eb573a0b36c99c5a2012f12319a548986
Gerrit-Change-Number: 14427
Gerrit-PatchSet: 2
Gerrit-Owner: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 15 Oct 2019 00:28:03 +0000
Gerrit-HasComments: Yes
[kudu-CR] Add a DATE type backed by INT32 (Part 1, C++ client)
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14427 )
Change subject: Add a DATE type backed by INT32 (Part 1, C++ client)
......................................................................
Patch Set 2:
(6 comments)
Looks good overall. I presume you basically looked at all the places Attila had to modify in his VARCHAR patch?
http://gerrit.cloudera.org:8080/#/c/14427/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14427/2//COMMIT_MSG@7
PS2, Line 7: Add a DATE type backed by INT32 (Part 1, C++ client)
Tag with KUDU-2632.
http://gerrit.cloudera.org:8080/#/c/14427/2//COMMIT_MSG@9
PS2, Line 9: This adds a new DATE type, represented by an INT32 and
Could you add some commentary about validation (or lack thereof)? For example, we could validate that the date range falls between 0001-01-01 and 9999-12-31, but doesn't look like this patch does that. Should it? Or not? Why?
http://gerrit.cloudera.org:8080/#/c/14427/2/src/kudu/common/partial_row.h
File src/kudu/common/partial_row.h:
http://gerrit.cloudera.org:8080/#/c/14427/2/src/kudu/common/partial_row.h@365
PS2, Line 365: Status GetUnixTimeMicros(const Slice& col_name,
: int64_t* micros_since_utc_epoch) const WARN_UNUSED_RESULT;
: Status GetDate(const Slice& col_name,
: int32_t* days_since_unix_epoch) const WARN_UNUSED_RESULT;
Nit: could you fix the indentation here?
http://gerrit.cloudera.org:8080/#/c/14427/2/src/kudu/common/row.h
File src/kudu/common/row.h:
http://gerrit.cloudera.org:8080/#/c/14427/2/src/kudu/common/row.h@669
PS2, Line 669: void AddDate(int32_t seconds_utc_since_epoch) {
Should be days_since, right?
http://gerrit.cloudera.org:8080/#/c/14427/2/src/kudu/common/types.h
File src/kudu/common/types.h:
http://gerrit.cloudera.org:8080/#/c/14427/2/src/kudu/common/types.h@590
PS2, Line 590: const int32_t *
Nit: "const int32_t*"
http://gerrit.cloudera.org:8080/#/c/14427/2/src/kudu/common/types.h@593
PS2, Line 593: gmtime_r(&days_since_unix_epoch, &tm_info);
I assume this does the right thing when days_since_unix_epoch is negative?
--
To view, visit http://gerrit.cloudera.org:8080/14427
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1d803b6eb573a0b36c99c5a2012f12319a548986
Gerrit-Change-Number: 14427
Gerrit-PatchSet: 2
Gerrit-Owner: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 14 Oct 2019 19:09:05 +0000
Gerrit-HasComments: Yes
[kudu-CR] [KUDU-2632] Add a DATE type backed by INT32 (Part 1, C++ client)
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14427 )
Change subject: [KUDU-2632] Add a DATE type backed by INT32 (Part 1, C++ client)
......................................................................
Patch Set 12: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14427/11/src/kudu/common/types-test.cc
File src/kudu/common/types-test.cc:
http://gerrit.cloudera.org:8080/#/c/14427/11/src/kudu/common/types-test.cc@56
PS11, Line 56: TestDateToString("1-01-01", *DataTypeTraits<DATE>::min_value());
: TestDateToString("9999-12-31", *DataTypeTraits<DATE>::max_value());
> Done
Thank you!
--
To view, visit http://gerrit.cloudera.org:8080/14427
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1d803b6eb573a0b36c99c5a2012f12319a548986
Gerrit-Change-Number: 14427
Gerrit-PatchSet: 12
Gerrit-Owner: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 17 Dec 2019 20:32:09 +0000
Gerrit-HasComments: Yes
[kudu-CR] [KUDU-2632] Add a DATE type backed by INT32 (Part 1, C++ client)
Posted by "Volodymyr Verovkin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, Greg Solovyev, Grant Henke,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14427
to look at the new patch set (#5).
Change subject: [KUDU-2632] Add a DATE type backed by INT32 (Part 1, C++ client)
......................................................................
[KUDU-2632] Add a DATE type backed by INT32 (Part 1, C++ client)
This adds a new DATE type, represented by an INT32 and
that should store the number of days from the Unix epoch, January 1,
1970.
Range:
0001-01-01 to 9999-12-31
or
-719162 to 2932896 (days; computed with mktime()/(24*60*60),
timezone = GMT, daylight saving = 0)
Range validation is done in KuduPartialRow::SetDate() function.
Change-Id: I1d803b6eb573a0b36c99c5a2012f12319a548986
---
M src/kudu/client/client-unittest.cc
M src/kudu/client/predicate-test.cc
M src/kudu/client/scan_batch.cc
M src/kudu/client/scan_batch.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/value.cc
M src/kudu/common/common.proto
M src/kudu/common/key_util.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/partition.cc
M src/kudu/common/row.h
M src/kudu/common/types-test.cc
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/hms/hms_catalog-test.cc
M src/kudu/hms/hms_catalog.cc
M src/kudu/integration-tests/all_types-itest.cc
M src/kudu/integration-tests/hms_itest-base.cc
M src/kudu/tools/data_gen_util.cc
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/tool_action_perf.cc
M src/kudu/tools/tool_action_table.cc
24 files changed, 224 insertions(+), 15 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/14427/5
--
To view, visit http://gerrit.cloudera.org:8080/14427
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1d803b6eb573a0b36c99c5a2012f12319a548986
Gerrit-Change-Number: 14427
Gerrit-PatchSet: 5
Gerrit-Owner: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
[kudu-CR] [KUDU-2632] Add a DATE type backed by INT32 (Part 1, C++ client)
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14427 )
Change subject: [KUDU-2632] Add a DATE type backed by INT32 (Part 1, C++ client)
......................................................................
Patch Set 3:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/14427/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14427/3//COMMIT_MSG@12
PS3, Line 12: Range:
Where is this enforced/validated?
http://gerrit.cloudera.org:8080/#/c/14427/3//COMMIT_MSG@13
PS3, Line 13: 0000-01-01 to 9999-12-31
See KUDU-2632: the SQL spec says we should start with 0001.
http://gerrit.cloudera.org:8080/#/c/14427/3//COMMIT_MSG@15
PS3, Line 15: -719528 to 2932896 (days; computed with mktime()/(24*60*60), timezone = GMT, daylight saving = 0)
Too long, wrap.
http://gerrit.cloudera.org:8080/#/c/14427/2/src/kudu/common/partial_row.h
File src/kudu/common/partial_row.h:
http://gerrit.cloudera.org:8080/#/c/14427/2/src/kudu/common/partial_row.h@365
PS2, Line 365: Status GetUnixTimeMicros(const Slice& col_name,
: int64_t* micros_since_utc_epoch) const WARN_UNUSED_RESULT;
: Status GetDate(const Slice& col_name,
: int32_t* days_since_unix_epoch) const WARN_UNUSED_RESULT;
> Done
Not done?
http://gerrit.cloudera.org:8080/#/c/14427/3/src/kudu/common/types.h
File src/kudu/common/types.h:
http://gerrit.cloudera.org:8080/#/c/14427/3/src/kudu/common/types.h@593
PS3, Line 593: static void AppendDebugStringForValue(const void* val, std::string* str) {
Could you simplify with StringAppendStrftime (from gutil/walltime.h)?
http://gerrit.cloudera.org:8080/#/c/14427/3/src/kudu/common/types.h@600
PS3, Line 600: if (rc == 0)
: str->append("invalid arguments for strftime()");
: else
: str->append(date);
Rewrite as a ternary:
str->append(rc == 0 ? "..." : date);
--
To view, visit http://gerrit.cloudera.org:8080/14427
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1d803b6eb573a0b36c99c5a2012f12319a548986
Gerrit-Change-Number: 14427
Gerrit-PatchSet: 3
Gerrit-Owner: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 16 Oct 2019 16:27:16 +0000
Gerrit-HasComments: Yes
[kudu-CR] [KUDU-2632] Add a DATE type backed by INT32 (Part 1, C++ client)
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/14427 )
Change subject: [KUDU-2632] Add a DATE type backed by INT32 (Part 1, C++ client)
......................................................................
Patch Set 3:
(8 comments)
Looks good. Mainly comments on properly handling DATE_MIN and DATE_MAX.
http://gerrit.cloudera.org:8080/#/c/14427/3/src/kudu/client/scan_batch.h
File src/kudu/client/scan_batch.h:
http://gerrit.cloudera.org:8080/#/c/14427/3/src/kudu/client/scan_batch.h@246
PS3, Line 246: Status GetDate(int col_idx, int32_t* days_since_unix_epoch) const WARN_UNUSED_RESULT;
nit: use days_since_utc_epoch to match GetUnixTimeMicros. Here and elsewhere too.
http://gerrit.cloudera.org:8080/#/c/14427/3/src/kudu/client/schema.h
File src/kudu/client/schema.h:
http://gerrit.cloudera.org:8080/#/c/14427/3/src/kudu/client/schema.h@192
PS3, Line 192: TIMESTAMP = UNIXTIME_MICROS, //!< deprecated, use UNIXTIME_MICROS
Nit: Keep timestamp last
http://gerrit.cloudera.org:8080/#/c/14427/3/src/kudu/common/partial_row.cc
File src/kudu/common/partial_row.cc:
http://gerrit.cloudera.org:8080/#/c/14427/3/src/kudu/common/partial_row.cc@265
PS3, Line 265: return Set<TypeTraits<DATE> >(col_name, days_since_unix_epoch);
Do we validate the range of values anywhere? Per the ANSI SQL spec, the valid date range is 0001-01-01 to 9999-12-31
http://gerrit.cloudera.org:8080/#/c/14427/3/src/kudu/common/partition.cc
File src/kudu/common/partition.cc:
http://gerrit.cloudera.org:8080/#/c/14427/3/src/kudu/common/partition.cc@1013
PS3, Line 1013: case DATE:
I think this should be DATE_MIN (0001-01-01) + 1 given the minimum and maximum date values allowed.
http://gerrit.cloudera.org:8080/#/c/14427/3/src/kudu/common/partition.cc@1100
PS3, Line 1100: if (value < INT32_MAX) {
I think this should be checking DATE_MAX (9999-12-3) given the minimum and maximum date values allowed.
http://gerrit.cloudera.org:8080/#/c/14427/3/src/kudu/common/types-test.cc
File src/kudu/common/types-test.cc:
http://gerrit.cloudera.org:8080/#/c/14427/3/src/kudu/common/types-test.cc@59
PS3, Line 59: TestDateToString("-5877641-06-23", std::numeric_limits<int32_t>::min());
These values should be invalid.
http://gerrit.cloudera.org:8080/#/c/14427/3/src/kudu/common/types.h
File src/kudu/common/types.h:
http://gerrit.cloudera.org:8080/#/c/14427/3/src/kudu/common/types.h@583
PS3, Line 583: constexpr static const char* kDateFormat = "%F"; // the ISO 8601 date format
Can the kDateFormat above be used?
http://gerrit.cloudera.org:8080/#/c/14427/3/src/kudu/common/types.h@584
PS3, Line 584: static constexpr time_t kSecondsInDay = 24 * 60 * 60;
This could be moved up by US_TO_S to keep conversions in the same place.
--
To view, visit http://gerrit.cloudera.org:8080/14427
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1d803b6eb573a0b36c99c5a2012f12319a548986
Gerrit-Change-Number: 14427
Gerrit-PatchSet: 3
Gerrit-Owner: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 16 Oct 2019 13:30:37 +0000
Gerrit-HasComments: Yes
[kudu-CR] [KUDU-2632] Add a DATE type backed by INT32 (Part 1, C++ client)
Posted by "Volodymyr Verovkin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, Greg Solovyev,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14427
to look at the new patch set (#3).
Change subject: [KUDU-2632] Add a DATE type backed by INT32 (Part 1, C++ client)
......................................................................
[KUDU-2632] Add a DATE type backed by INT32 (Part 1, C++ client)
This adds a new DATE type, represented by an INT32 and
that should store the number of days from the Unix epoch, January 1,
1970.
Range:
0000-01-01 to 9999-12-31
or
-719528 to 2932896 (days; computed with mktime()/(24*60*60), timezone = GMT, daylight saving = 0)
Change-Id: I1d803b6eb573a0b36c99c5a2012f12319a548986
---
M src/kudu/client/client-unittest.cc
M src/kudu/client/predicate-test.cc
M src/kudu/client/scan_batch.cc
M src/kudu/client/scan_batch.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/value.cc
M src/kudu/common/common.proto
M src/kudu/common/key_util.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/partition.cc
M src/kudu/common/row.h
M src/kudu/common/types-test.cc
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/hms/hms_catalog-test.cc
M src/kudu/hms/hms_catalog.cc
M src/kudu/integration-tests/all_types-itest.cc
M src/kudu/integration-tests/hms_itest-base.cc
M src/kudu/tools/data_gen_util.cc
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/tool_action_perf.cc
M src/kudu/tools/tool_action_table.cc
24 files changed, 194 insertions(+), 8 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/14427/3
--
To view, visit http://gerrit.cloudera.org:8080/14427
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1d803b6eb573a0b36c99c5a2012f12319a548986
Gerrit-Change-Number: 14427
Gerrit-PatchSet: 3
Gerrit-Owner: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [KUDU-2632] Add a DATE type backed by INT32 (Part 1, C++ client)
Posted by "Volodymyr Verovkin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, Greg Solovyev, Grant Henke,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14427
to look at the new patch set (#12).
Change subject: [KUDU-2632] Add a DATE type backed by INT32 (Part 1, C++ client)
......................................................................
[KUDU-2632] Add a DATE type backed by INT32 (Part 1, C++ client)
This adds a new DATE type, represented by an INT32 and
that should store the number of days from the Unix epoch, January 1,
1970.
Range:
0001-01-01 to 9999-12-31
or
-719162 to 2932896 (days; computed with mktime()/(24*60*60),
timezone = GMT, daylight saving = 0)
Range validation is done in KuduPartialRow::SetDate() function.
Change-Id: I1d803b6eb573a0b36c99c5a2012f12319a548986
---
M src/kudu/client/client-unittest.cc
M src/kudu/client/predicate-test.cc
M src/kudu/client/scan_batch.cc
M src/kudu/client/scan_batch.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/value.cc
M src/kudu/common/common.proto
M src/kudu/common/key_util.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/partition.cc
M src/kudu/common/row.h
M src/kudu/common/types-test.cc
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/hms/hms_catalog-test.cc
M src/kudu/hms/hms_catalog.cc
M src/kudu/integration-tests/all_types-itest.cc
M src/kudu/integration-tests/data_gen_util.cc
M src/kudu/integration-tests/hms_itest-base.cc
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/tool_action_perf.cc
M src/kudu/tools/tool_action_table.cc
24 files changed, 315 insertions(+), 25 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/14427/12
--
To view, visit http://gerrit.cloudera.org:8080/14427
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1d803b6eb573a0b36c99c5a2012f12319a548986
Gerrit-Change-Number: 14427
Gerrit-PatchSet: 12
Gerrit-Owner: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
[kudu-CR] [KUDU-2632] Add a DATE type backed by INT32 (Part 1, C++ client)
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14427 )
Change subject: [KUDU-2632] Add a DATE type backed by INT32 (Part 1, C++ client)
......................................................................
Patch Set 7:
(5 comments)
Few nits
http://gerrit.cloudera.org:8080/#/c/14427/11/src/kudu/common/partial_row.h
File src/kudu/common/partial_row.h:
http://gerrit.cloudera.org:8080/#/c/14427/11/src/kudu/common/partial_row.h@450
PS11, Line 450:
style nit: if continuing a statement on the next line, shift is two times of the standard scope shift, i.e. this should be 4, not 2 spaces
http://gerrit.cloudera.org:8080/#/c/14427/11/src/kudu/common/partial_row.cc
File src/kudu/common/partial_row.cc:
http://gerrit.cloudera.org:8080/#/c/14427/11/src/kudu/common/partial_row.cc@50
PS11, Line 50: const Schema* schema
style nit: I'm not sure we widely use constant pointers as input parameters. Why not to use constant reference here instead?
http://gerrit.cloudera.org:8080/#/c/14427/11/src/kudu/common/partial_row.cc@715
PS11, Line 715:
nit: fix the alignment
http://gerrit.cloudera.org:8080/#/c/14427/11/src/kudu/common/types-test.cc
File src/kudu/common/types-test.cc:
http://gerrit.cloudera.org:8080/#/c/14427/11/src/kudu/common/types-test.cc@56
PS11, Line 56: TestDateToString("1970-01-01", 0);
: TestDateToString("1942-08-16", -10000);
Does it make sense to add a couple more cases to test for off-by-one issues, if any? Something like this:
TestDateToString("value ...", *DataTypeTraits<DATE>::min_value() - 1);
TestDateToString("value ...", *DataTypeTraits<DATE>::max_value() + 1);
http://gerrit.cloudera.org:8080/#/c/14427/11/src/kudu/common/types.h
File src/kudu/common/types.h:
http://gerrit.cloudera.org:8080/#/c/14427/11/src/kudu/common/types.h@604
PS11, Line 604:
nit: remove the extra line
--
To view, visit http://gerrit.cloudera.org:8080/14427
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1d803b6eb573a0b36c99c5a2012f12319a548986
Gerrit-Change-Number: 14427
Gerrit-PatchSet: 7
Gerrit-Owner: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Fri, 13 Dec 2019 06:11:42 +0000
Gerrit-HasComments: Yes
[kudu-CR] [KUDU-2632] Add a DATE type backed by INT32 (Part 1, C++ client)
Posted by "Volodymyr Verovkin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, Greg Solovyev, Grant Henke,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14427
to look at the new patch set (#6).
Change subject: [KUDU-2632] Add a DATE type backed by INT32 (Part 1, C++ client)
......................................................................
[KUDU-2632] Add a DATE type backed by INT32 (Part 1, C++ client)
This adds a new DATE type, represented by an INT32 and
that should store the number of days from the Unix epoch, January 1,
1970.
Range:
0001-01-01 to 9999-12-31
or
-719162 to 2932896 (days; computed with mktime()/(24*60*60),
timezone = GMT, daylight saving = 0)
Range validation is done in KuduPartialRow::SetDate() function.
Change-Id: I1d803b6eb573a0b36c99c5a2012f12319a548986
---
M src/kudu/client/client-unittest.cc
M src/kudu/client/predicate-test.cc
M src/kudu/client/scan_batch.cc
M src/kudu/client/scan_batch.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/value.cc
M src/kudu/common/common.proto
M src/kudu/common/key_util.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/partition.cc
M src/kudu/common/row.h
M src/kudu/common/types-test.cc
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/hms/hms_catalog-test.cc
M src/kudu/hms/hms_catalog.cc
M src/kudu/integration-tests/all_types-itest.cc
M src/kudu/integration-tests/data_gen_util.cc
M src/kudu/integration-tests/hms_itest-base.cc
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/tool_action_perf.cc
M src/kudu/tools/tool_action_table.cc
24 files changed, 223 insertions(+), 15 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/14427/6
--
To view, visit http://gerrit.cloudera.org:8080/14427
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1d803b6eb573a0b36c99c5a2012f12319a548986
Gerrit-Change-Number: 14427
Gerrit-PatchSet: 6
Gerrit-Owner: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>