impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5572: Timestamp codegen for text scanner
Date Tue, 01 Aug 2017 23:11:28 GMT
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5572: Timestamp codegen for text scanner
......................................................................


Patch Set 1:

(6 comments)

Nice! No major concerns, mostly comments about comments.

http://gerrit.cloudera.org:8080/#/c/7556/1//COMMIT_MSG
Commit Message:

Line 14: 
Can you add a short Testing: section here.

I'm thinking it would make sense to run all the tests on exhaustive, or at least the scanner
ones. There are some interesting timestamp-related tests in test_scanners, e.g. out-of-range
timestamps. You can run test_scanners.py with the exhaustive combinations of options like
this:

  impala-py.test tests/query_test/test_scanners.py --workload_exploration_strategy=functional:exhaustive
-n 5 --verbose


http://gerrit.cloudera.org:8080/#/c/7556/1/be/src/exec/hdfs-scanner-ir.cc
File be/src/exec/hdfs-scanner-ir.cc:

Line 119: TimestampValue IrStringToTimestamp(const char* s, int len,
It looks like the other functions have extern "C" to avoid mangling the function names. Does
that not work for functions returning TimestampValue?

No big deal, I'm mainly curious.


PS1, Line 120:  
Extra space before *


http://gerrit.cloudera.org:8080/#/c/7556/1/be/src/exec/text-converter.cc
File be/src/exec/text-converter.cc:

PS1, Line 246: 16Bytes
16 bytes


PS1, Line 268: mov
nit: move


http://gerrit.cloudera.org:8080/#/c/7556/1/be/src/util/string-parser.h
File be/src/util/string-parser.h:

Line 100:   /// Parse a TimestampValue from s
Nit: add a . on the end for consistency with other comments.


-- 
To view, visit http://gerrit.cloudera.org:8080/7556
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I00cbf8ec7784ca9594e14e952f46dc54a5ede44b
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang <twang@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message