Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201611211943.uALJhRqP022812@ip-10-146-233-104.ec2.internal>
Date: Mon, 21 Nov 2016 19:43:27 +0000
From: "Dan Hecht (Code Review)" <gerrit@cloudera.org>
To: Taras Bobrovytsky <tbobrovytsky@cloudera.com>, impala-cr@cloudera.com,
        reviews@impala.incubator.apache.org
CC: Lars Volker <lv@cloudera.com>, Laszlo Gaal <laszlo.gaal@cloudera.com>,
        Alex Behm <alex.behm@cloudera.com>
Reply-To: dhecht@cloudera.com
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-4363=3A_Add_Parquet_timestamp_validation=0A?=
In-Reply-To: <gerrit.1478378647000.I9988449aa0dc0f39fabb91ce6cce0dd8a06e8bcf@gerrit.cloudera.org>
References: <gerrit.1478378647000.I9988449aa0dc0f39fabb91ce6cce0dd8a06e8bcf@gerrit.cloudera.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.12.2
archived-at: Mon, 21 Nov 2016 19:44:35 -0000

Dan Hecht has posted comments on this change.

Change subject: IMPALA-4363: Add Parquet timestamp validation
......................................................................


Patch Set 7:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/4968/7/be/src/exec/parquet-column-readers.cc
File be/src/exec/parquet-column-readers.cc:

Line 473:     ErrorMsg msg;
this has a non-trivial destructor, so we should probably only construct it after checking NeedsValidation().  Also, why are NeedsValidation() and NeedsConversion() mutually exclusive?  Actually, doesn't this break --convert_legacy_hive_parquet_utc_timestamps since we'll never take the conversion path for timestamp now? (Do we not have tests for that?)  That is, I think both NeedsConversion() and NeedsValidation() can be true, and the code needs to be adjusted for that.

Also consider tucking lines 475 to 480 inside ValidateSlot() and that way the ErrorMsg doesn't have to be passed back and forth, and it'll naturally keep the construction after the NeedsValidation() check.


PS7, Line 474: val_ptr
you'll also have to be careful that we pass the right pointer here, depending on how you address the comment above.


http://gerrit.cloudera.org:8080/#/c/4968/7/be/src/runtime/timestamp-value.h
File be/src/runtime/timestamp-value.h:

Line 147:   /// Verifies that the timestamp date falls into a valid range (years 1400..9999).
update this comment to be clear that we also validate time.


PS7, Line 156: l
doesn't this have to be "ll" (long long)?


PS7, Line 160: time_.ticks() < NUMBER_OF_NANOSECONDS_IN_24H
I'm slightly worried about how this interacts with leap seconds.  Are you sure this is always illegal?  If it doesn't crash, what behavior did you see (i.e. how do you know this is the correct bounds)?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9988449aa0dc0f39fabb91ce6cce0dd8a06e8bcf
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tbobrovytsky@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Laszlo Gaal <laszlo.gaal@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tbobrovytsky@cloudera.com>
Gerrit-HasComments: Yes