impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex Behm (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
Date Wed, 19 Apr 2017 00:36:09 GMT
Alex Behm has posted comments on this change.

Change subject: IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
......................................................................


Patch Set 2: Code-Review+1

(3 comments)

http://gerrit.cloudera.org:8080/#/c/6610/2/be/src/runtime/descriptors.cc
File be/src/runtime/descriptors.cc:

Line 347:   vector<SlotDescriptor*> slots = this->slots();
can remove this->


Line 353:   // slots on 'slot_idx'. We define a struct that wraps the comparison function.
Can remove the last sentence (obvious from code)


http://gerrit.cloudera.org:8080/#/c/6610/2/testdata/workloads/functional-query/queries/QueryTest/union.test
File testdata/workloads/functional-query/queries/QueryTest/union.test:

Line 1125: # IMPALA-5188: The slot descriptors might be ordered differently for the operands
and the
> Several queries were already added as part of the union passthrough patch (
This is a very specific bug. Imo, we should check in the min repro of a bug, and not necessarily
the original query that found it. Of course, we need to be sufficiently confident that the
simplified query covers the same issue, in particular, we should manually test that the original
query is fixed by this patch.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I426ad244678dbfe517262dfb7bbf4adc0247a35e
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tbobrovytsky@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Michael Brown <mikeb@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tbobrovytsky@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message