You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org> on 2017/04/12 01:15:24 UTC
[Impala-ASF-CR] IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
Taras Bobrovytsky has uploaded a new change for review.
http://gerrit.cloudera.org:8080/6610
Change subject: IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
......................................................................
IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
The slot descriptor vectors are not guaranteed to be sorted on the slot
index within a tuple. As a result, TupleDescriptor::LayoutEquals()
sometimes returned a wrong result.
In this patch, we sort the vectors of slot descriptors on the slot index
within the tuple before comparing the vectors.
Testing:
- ran EE tests locally.
Change-Id: I426ad244678dbfe517262dfb7bbf4adc0247a35e
---
M be/src/runtime/descriptors.cc
M be/src/runtime/descriptors.h
M testdata/workloads/functional-query/queries/QueryTest/union.test
3 files changed, 43 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/10/6610/1
--
To view, visit http://gerrit.cloudera.org:8080/6610
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I426ad244678dbfe517262dfb7bbf4adc0247a35e
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
[Impala-ASF-CR] IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Hello Impala Public Jenkins, Alex Behm,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/6610
to look at the new patch set (#7).
Change subject: IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
......................................................................
IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
The slot descriptor vectors are not guaranteed to be sorted on the slot
index within a tuple. As a result, TupleDescriptor::LayoutEquals()
sometimes returned a wrong result.
In this patch, we sort the vectors of slot descriptors on the slot index
within the tuple before comparing the vectors.
Testing:
- ran EE tests locally.
Change-Id: I426ad244678dbfe517262dfb7bbf4adc0247a35e
---
M be/src/runtime/descriptors.cc
M be/src/runtime/descriptors.h
M fe/src/main/java/org/apache/impala/analysis/TupleDescriptor.java
M fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java
M testdata/workloads/functional-query/queries/QueryTest/union.test
5 files changed, 51 insertions(+), 8 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/10/6610/7
--
To view, visit http://gerrit.cloudera.org:8080/6610
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I426ad244678dbfe517262dfb7bbf4adc0247a35e
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
[Impala-ASF-CR] IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Taras Bobrovytsky has uploaded a new patch set (#2).
Change subject: IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
......................................................................
IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
The slot descriptor vectors are not guaranteed to be sorted on the slot
index within a tuple. As a result, TupleDescriptor::LayoutEquals()
sometimes returned a wrong result.
In this patch, we sort the vectors of slot descriptors on the slot index
within the tuple before comparing the vectors.
Testing:
- ran EE tests locally.
Change-Id: I426ad244678dbfe517262dfb7bbf4adc0247a35e
---
M be/src/runtime/descriptors.cc
M be/src/runtime/descriptors.h
M testdata/workloads/functional-query/queries/QueryTest/union.test
3 files changed, 44 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/10/6610/2
--
To view, visit http://gerrit.cloudera.org:8080/6610
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I426ad244678dbfe517262dfb7bbf4adc0247a35e
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
[Impala-ASF-CR] IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
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 <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
Posted by "Michael Brown (Code Review)" <ge...@cloudera.org>.
Michael Brown has posted comments on this change.
Change subject: IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
......................................................................
Patch Set 2:
(1 comment)
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
Question more than comment: The query here is a simplified version of one found by the query generator. But does this regression test query cover all the cases? Would more queries need to be added here to enhance regression testing of UNION passthrough?
--
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 <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5188: Sort the slots in a tuple before serializing to Thrift
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Taras Bobrovytsky has posted comments on this change.
Change subject: IMPALA-5188: Sort the slots in a tuple before serializing to Thrift
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6610/1/be/src/runtime/descriptors.cc
File be/src/runtime/descriptors.cc:
Line 344: // The vectors get copied because we intend to sort them and it is better to keep this
> How about solving this in the FE?
It doesn't quite work. The corresponding expr trees need to be in the same order too.
Otherwise it hits a DCHECK: tuple.cc:379] Check failed: slot_desc->type().type == TYPE_NULL || slot_desc->type() == materialize_expr_ctxs[i]->root()->type()
Does it make sense to try to sort the exprs too?
--
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: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
......................................................................
Patch Set 6: Verified-1
Build failed: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/506/
--
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: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
......................................................................
Patch Set 4:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/6610/1/be/src/runtime/descriptors.cc
File be/src/runtime/descriptors.cc:
Line 344: // Some exec nodes rely on the slot-order in the tuple descriptor to correspond to the
> The agg node also relies on this:
Hmm, i see.
http://gerrit.cloudera.org:8080/#/c/6610/4/be/src/runtime/descriptors.cc
File be/src/runtime/descriptors.cc:
Line 346: // originals.
i think a comment like this would be better at the slots_ field definition. something like:
/// Slots are in the same order as the expressions that materialize them. See Tuple::MaterializeExprs().
Line 360: std::sort(other_slots.begin(), other_slots.end(), SlotComparator());
how about using the same sort technique we use in TupleDescriptor::GetLlvmStruct()? you could even factor that into a private TupleDescriptor::GetSortedSlots() subroutine.
--
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: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Taras Bobrovytsky has posted comments on this change.
Change subject: IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
......................................................................
Patch Set 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6610/7/be/src/runtime/descriptors.cc
File be/src/runtime/descriptors.cc:
Line 529: }
> let's verify (DCHECK) that all tuples are dense at this point.
After implementing this DCHECK, it turns out we hit it quite often. The reason is we sometimes send materialized tuples without a mem layout to the BE (which are basically unusable) eg, when replacing plan trees with an EmptySetNode.
There is a TODO in DescriptorTable.toThrift() regarding this.
--
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: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Taras Bobrovytsky has posted comments on this change.
Change subject: IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
......................................................................
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6610/6/be/src/runtime/descriptors.cc
File be/src/runtime/descriptors.cc:
Line 678: for (SlotDescriptor* slot: slots()) sorted_slots[slot->slot_idx_] = slot;
It turns out that the reason why the GVO for this patch is failing is that slot_idx does not always start with 0. The following query produces a tuple with a single slot with a slot_idx=1:
select c_custkey, v1.cnt
from tpch_nested_parquet.customer c
inner join
(select count(*) cnt from c.c_orders
where false) v1
where c_custkey < 10
The tuple looks like this:
Tuple(id=0 size=25 slots=[Slot(id=3 type=BIGINT col_path=[0] offset=16 null=(offset=24 mask=2) slot_idx=1 field_idx=-1)] tuple_path=[])
--
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: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Hello Alex Behm,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/6610
to look at the new patch set (#3).
Change subject: IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
......................................................................
IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
The slot descriptor vectors are not guaranteed to be sorted on the slot
index within a tuple. As a result, TupleDescriptor::LayoutEquals()
sometimes returned a wrong result.
In this patch, we sort the vectors of slot descriptors on the slot index
within the tuple before comparing the vectors.
Testing:
- ran EE tests locally.
Change-Id: I426ad244678dbfe517262dfb7bbf4adc0247a35e
---
M be/src/runtime/descriptors.cc
M be/src/runtime/descriptors.h
M testdata/workloads/functional-query/queries/QueryTest/union.test
3 files changed, 44 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/10/6610/3
--
To view, visit http://gerrit.cloudera.org:8080/6610
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I426ad244678dbfe517262dfb7bbf4adc0247a35e
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
[Impala-ASF-CR] IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Hello Alex Behm,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/6610
to look at the new patch set (#6).
Change subject: IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
......................................................................
IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
The slot descriptor vectors are not guaranteed to be sorted on the slot
index within a tuple. As a result, TupleDescriptor::LayoutEquals()
sometimes returned a wrong result.
In this patch, we sort the vectors of slot descriptors on the slot index
within the tuple before comparing the vectors.
Testing:
- ran EE tests locally.
Change-Id: I426ad244678dbfe517262dfb7bbf4adc0247a35e
---
M be/src/runtime/descriptors.cc
M be/src/runtime/descriptors.h
M testdata/workloads/functional-query/queries/QueryTest/union.test
3 files changed, 40 insertions(+), 8 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/10/6610/6
--
To view, visit http://gerrit.cloudera.org:8080/6610
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I426ad244678dbfe517262dfb7bbf4adc0247a35e
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
[Impala-ASF-CR] IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6610/4/be/src/runtime/descriptors.cc
File be/src/runtime/descriptors.cc:
Line 360: std::sort(other_slots.begin(), other_slots.end(), SlotComparator());
> how about using the same sort technique we use in TupleDescriptor::GetLlvmS
Or maybe TupleDescriptor::SlotsOrderedbyIdx()?
--
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: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Taras Bobrovytsky has uploaded a new patch set (#6).
Change subject: IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
......................................................................
IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
The slot descriptor vectors are not guaranteed to be sorted on the slot
index within a tuple. As a result, TupleDescriptor::LayoutEquals()
sometimes returned a wrong result.
In this patch, we sort the vectors of slot descriptors on the slot index
within the tuple before comparing the vectors.
Testing:
- ran EE tests locally.
Change-Id: I426ad244678dbfe517262dfb7bbf4adc0247a35e
---
M be/src/runtime/descriptors.cc
M be/src/runtime/descriptors.h
M testdata/workloads/functional-query/queries/QueryTest/union.test
3 files changed, 39 insertions(+), 8 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/10/6610/6
--
To view, visit http://gerrit.cloudera.org:8080/6610
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I426ad244678dbfe517262dfb7bbf4adc0247a35e
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
[Impala-ASF-CR] IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged.
Change subject: IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
......................................................................
IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
The slot descriptor vectors are not guaranteed to be sorted on the slot
index within a tuple. As a result, TupleDescriptor::LayoutEquals()
sometimes returned a wrong result.
In this patch, we sort the vectors of slot descriptors on the slot index
within the tuple before comparing the vectors.
Testing:
- ran EE tests locally.
Change-Id: I426ad244678dbfe517262dfb7bbf4adc0247a35e
Reviewed-on: http://gerrit.cloudera.org:8080/6610
Reviewed-by: Dan Hecht <dh...@cloudera.com>
Reviewed-by: Alex Behm <al...@cloudera.com>
Tested-by: Impala Public Jenkins
---
M be/src/runtime/descriptors.cc
M be/src/runtime/descriptors.h
M fe/src/main/java/org/apache/impala/analysis/TupleDescriptor.java
M fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java
M testdata/workloads/functional-query/queries/QueryTest/union.test
5 files changed, 51 insertions(+), 8 deletions(-)
Approvals:
Impala Public Jenkins: Verified
Alex Behm: Looks good to me, approved
Dan Hecht: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/6610
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I426ad244678dbfe517262dfb7bbf4adc0247a35e
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
[Impala-ASF-CR] IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Taras Bobrovytsky has posted comments on this change.
Change subject: IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
......................................................................
Patch Set 4:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/6610/4/be/src/runtime/descriptors.cc
File be/src/runtime/descriptors.cc:
Line 346: // originals.
> i think a comment like this would be better at the slots_ field definition.
Removed the comment here and added the comment you suggested to the slots_ field definition.
Line 360: std::sort(other_slots.begin(), other_slots.end(), SlotComparator());
> Or maybe TupleDescriptor::SlotsOrderedbyIdx()?
Done, refactored, also removed comment and DCHECKS.
--
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: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Hello Alex Behm,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/6610
to look at the new patch set (#5).
Change subject: IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
......................................................................
IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
The slot descriptor vectors are not guaranteed to be sorted on the slot
index within a tuple. As a result, TupleDescriptor::LayoutEquals()
sometimes returned a wrong result.
In this patch, we sort the vectors of slot descriptors on the slot index
within the tuple before comparing the vectors.
Testing:
- ran EE tests locally.
Change-Id: I426ad244678dbfe517262dfb7bbf4adc0247a35e
---
M be/src/runtime/descriptors.cc
M be/src/runtime/descriptors.h
M testdata/workloads/functional-query/queries/QueryTest/union.test
3 files changed, 40 insertions(+), 8 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/10/6610/5
--
To view, visit http://gerrit.cloudera.org:8080/6610
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I426ad244678dbfe517262dfb7bbf4adc0247a35e
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
[Impala-ASF-CR] IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
......................................................................
Patch Set 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6610/7/be/src/runtime/descriptors.cc
File be/src/runtime/descriptors.cc:
Line 529: }
> Hmm, okay. That's too bad
The FE does some pretty hairy stuff with tuples, look at an interesting TODO/comment in DescriptorTable.toThrift().
Basically, the FE sometimes sends unused tuples in an unusual state to the BE.
--
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: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5188: Sort the slots in a tuple before serializing to Thrift
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-5188: Sort the slots in a tuple before serializing to Thrift
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6610/1/be/src/runtime/descriptors.cc
File be/src/runtime/descriptors.cc:
Line 344: // The vectors get copied because we intend to sort them and it is better to keep this
> It doesn't quite work. The corresponding expr trees need to be in the same
Not sure I understand the problem, let's chat about it tomorrow. Are you saying the code in the latest patch set does not solve the problem?
--
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: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Taras Bobrovytsky has posted comments on this change.
Change subject: IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6610/1/be/src/runtime/descriptors.cc
File be/src/runtime/descriptors.cc:
Line 344: // The vectors get copied because we intend to sort them and it is better to keep this
> Can you point out an example of where that assumption is used? This all see
Well, for example it's used here:
http://github.mtv.cloudera.com/CDH/Impala/blob/cdh5-trunk/be/src/exec/topn-node-ir.cc#L40
The expressions here: sort_exec_exprs_.sort_tuple_slot_expr_ctxs() should be in the same order as the slots in output tuple.
This causes a DCHECK in topn codegen:
Otherwise it hits a DCHECK: tuple.cc:379] Check failed: slot_desc->type().type == TYPE_NULL || slot_desc->type() == materialize_expr_ctxs[i]->root()->type()
--
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: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Taras Bobrovytsky has uploaded a new patch set (#4).
Change subject: IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
......................................................................
IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
The slot descriptor vectors are not guaranteed to be sorted on the slot
index within a tuple. As a result, TupleDescriptor::LayoutEquals()
sometimes returned a wrong result.
In this patch, we sort the vectors of slot descriptors on the slot index
within the tuple before comparing the vectors.
Testing:
- ran EE tests locally.
Change-Id: I426ad244678dbfe517262dfb7bbf4adc0247a35e
---
M be/src/runtime/descriptors.cc
M be/src/runtime/descriptors.h
M testdata/workloads/functional-query/queries/QueryTest/union.test
3 files changed, 44 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/10/6610/4
--
To view, visit http://gerrit.cloudera.org:8080/6610
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I426ad244678dbfe517262dfb7bbf4adc0247a35e
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
[Impala-ASF-CR] IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6610/1/be/src/runtime/descriptors.cc
File be/src/runtime/descriptors.cc:
Line 344: // The vectors get copied because we intend to sort them and it is better to keep this
How about solving this in the FE?
We already have a function TupleDescriptor.getSlotsOrderedByOffset() and you can use that in DescriptorTable.toThrift() in L187.
--
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: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Hello Alex Behm,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/6610
to look at the new patch set (#6).
Change subject: IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
......................................................................
IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
The slot descriptor vectors are not guaranteed to be sorted on the slot
index within a tuple. As a result, TupleDescriptor::LayoutEquals()
sometimes returned a wrong result.
In this patch, we sort the vectors of slot descriptors on the slot index
within the tuple before comparing the vectors.
Testing:
- ran EE tests locally.
Change-Id: I426ad244678dbfe517262dfb7bbf4adc0247a35e
---
M be/src/runtime/descriptors.cc
M be/src/runtime/descriptors.h
M testdata/workloads/functional-query/queries/QueryTest/union.test
3 files changed, 39 insertions(+), 8 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/10/6610/6
--
To view, visit http://gerrit.cloudera.org:8080/6610
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I426ad244678dbfe517262dfb7bbf4adc0247a35e
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
[Impala-ASF-CR] IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Taras Bobrovytsky has uploaded a new patch set (#2).
Change subject: IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
......................................................................
IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
The slot descriptor vectors are not guaranteed to be sorted on the slot
index within a tuple. As a result, TupleDescriptor::LayoutEquals()
sometimes returned a wrong result.
In this patch, we sort the vectors of slot descriptors on the slot index
within the tuple before comparing the vectors.
Testing:
- ran EE tests locally.
Change-Id: I426ad244678dbfe517262dfb7bbf4adc0247a35e
---
M be/src/runtime/descriptors.cc
M be/src/runtime/descriptors.h
M testdata/workloads/functional-query/queries/QueryTest/union.test
3 files changed, 44 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/10/6610/2
--
To view, visit http://gerrit.cloudera.org:8080/6610
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I426ad244678dbfe517262dfb7bbf4adc0247a35e
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
[Impala-ASF-CR] IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Hello Alex Behm,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/6610
to look at the new patch set (#6).
Change subject: IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
......................................................................
IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
The slot descriptor vectors are not guaranteed to be sorted on the slot
index within a tuple. As a result, TupleDescriptor::LayoutEquals()
sometimes returned a wrong result.
In this patch, we sort the vectors of slot descriptors on the slot index
within the tuple before comparing the vectors.
Testing:
- ran EE tests locally.
Change-Id: I426ad244678dbfe517262dfb7bbf4adc0247a35e
---
M be/src/runtime/descriptors.cc
M be/src/runtime/descriptors.h
M testdata/workloads/functional-query/queries/QueryTest/union.test
3 files changed, 39 insertions(+), 8 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/10/6610/6
--
To view, visit http://gerrit.cloudera.org:8080/6610
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I426ad244678dbfe517262dfb7bbf4adc0247a35e
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
[Impala-ASF-CR] IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6610/1/be/src/runtime/descriptors.cc
File be/src/runtime/descriptors.cc:
Line 344: // The vectors get copied because we intend to sort them and it is better to keep this
> Well, for example it's used here:
The agg node also relies on this:
http://github.mtv.cloudera.com/CDH/Impala/blob/cdh5-trunk/be/src/exec/partitioned-aggregation-node.cc#L219
--
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: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5188: Sort the slots in a tuple before serializing to Thrift
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Taras Bobrovytsky has uploaded a new patch set (#3).
Change subject: IMPALA-5188: Sort the slots in a tuple before serializing to Thrift
......................................................................
IMPALA-5188: Sort the slots in a tuple before serializing to Thrift
The slot descriptor vectors are not guaranteed to be sorted on the slot
index within a tuple. As a result, TupleDescriptor::LayoutEquals()
sometimes returned a wrong result.
In this patch, we sort the slot descriptors in the FE before
serializing to Thrift.
Testing:
- ran EE tests locally.
Change-Id: I426ad244678dbfe517262dfb7bbf4adc0247a35e
---
M be/src/runtime/descriptors.cc
M be/src/runtime/descriptors.h
M fe/src/main/java/org/apache/impala/analysis/DescriptorTable.java
M fe/src/main/java/org/apache/impala/analysis/TupleDescriptor.java
M fe/src/main/java/org/apache/impala/planner/KuduScanNode.java
M testdata/workloads/functional-query/queries/QueryTest/union.test
6 files changed, 30 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/10/6610/3
--
To view, visit http://gerrit.cloudera.org:8080/6610
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I426ad244678dbfe517262dfb7bbf4adc0247a35e
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
[Impala-ASF-CR] IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
......................................................................
Patch Set 7:
Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/533/
--
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: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Hello Alex Behm,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/6610
to look at the new patch set (#4).
Change subject: IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
......................................................................
IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
The slot descriptor vectors are not guaranteed to be sorted on the slot
index within a tuple. As a result, TupleDescriptor::LayoutEquals()
sometimes returned a wrong result.
In this patch, we sort the vectors of slot descriptors on the slot index
within the tuple before comparing the vectors.
Testing:
- ran EE tests locally.
Change-Id: I426ad244678dbfe517262dfb7bbf4adc0247a35e
---
M be/src/runtime/descriptors.cc
M be/src/runtime/descriptors.h
M testdata/workloads/functional-query/queries/QueryTest/union.test
3 files changed, 44 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/10/6610/4
--
To view, visit http://gerrit.cloudera.org:8080/6610
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I426ad244678dbfe517262dfb7bbf4adc0247a35e
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
[Impala-ASF-CR] IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
......................................................................
Patch Set 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6610/7/be/src/runtime/descriptors.cc
File be/src/runtime/descriptors.cc:
Line 529: }
> The FE does some pretty hairy stuff with tuples, look at an interesting TOD
Yeah, i saw that comment. It would be nice to clean up but not for 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: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
......................................................................
Patch Set 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6610/7/be/src/runtime/descriptors.cc
File be/src/runtime/descriptors.cc:
Line 529: }
let's verify (DCHECK) that all tuples are dense at this point.
--
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: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
......................................................................
Patch Set 6: Code-Review+2
--
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: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
......................................................................
Patch Set 7: Code-Review+2
(1 comment)
Carry +2 given Alex signed off on the new fe changes.
http://gerrit.cloudera.org:8080/#/c/6610/7/be/src/runtime/descriptors.cc
File be/src/runtime/descriptors.cc:
Line 529: }
> After implementing this DCHECK, it turns out we hit it quite often. The rea
Hmm, okay. That's too bad
--
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: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
......................................................................
Patch Set 7: Verified+1
--
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: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
......................................................................
Patch Set 5:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/6610/5/be/src/runtime/descriptors.h
File be/src/runtime/descriptors.h:
Line 131: int slot_idx() const { return slot_idx_; }
this doesn't need to be exposed, so let's remove it. TupleDescriptor is already a friend class.
Line 426: vector<SlotDescriptor*> SlotsOrderedByIdx() const;
this shouldn't be pubic either.
--
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: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5188: Sort the slots in a tuple before serializing to Thrift
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-5188: Sort the slots in a tuple before serializing to Thrift
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6610/1/be/src/runtime/descriptors.cc
File be/src/runtime/descriptors.cc:
Line 344: // The vectors get copied because we intend to sort them and it is better to keep this
> Not sure I understand the problem, let's chat about it tomorrow. Are you sa
Taras and I talked, and the reason why sorting in the FE does not work is that some BE nodes rely on the slot-order in the tuple descriptor to correspond to the order of some of their exprs. So looks like we're back to sorting on-the-fly in the BE.
--
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: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Hello Alex Behm,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/6610
to look at the new patch set (#5).
Change subject: IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
......................................................................
IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
The slot descriptor vectors are not guaranteed to be sorted on the slot
index within a tuple. As a result, TupleDescriptor::LayoutEquals()
sometimes returned a wrong result.
In this patch, we sort the vectors of slot descriptors on the slot index
within the tuple before comparing the vectors.
Testing:
- ran EE tests locally.
Change-Id: I426ad244678dbfe517262dfb7bbf4adc0247a35e
---
M be/src/runtime/descriptors.cc
M be/src/runtime/descriptors.h
M testdata/workloads/functional-query/queries/QueryTest/union.test
3 files changed, 39 insertions(+), 8 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/10/6610/5
--
To view, visit http://gerrit.cloudera.org:8080/6610
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I426ad244678dbfe517262dfb7bbf4adc0247a35e
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
[Impala-ASF-CR] IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
......................................................................
Patch Set 6:
Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/515/
--
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: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Taras Bobrovytsky has uploaded a new patch set (#7).
Change subject: IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
......................................................................
IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
The slot descriptor vectors are not guaranteed to be sorted on the slot
index within a tuple. As a result, TupleDescriptor::LayoutEquals()
sometimes returned a wrong result.
In this patch, we sort the vectors of slot descriptors on the slot index
within the tuple before comparing the vectors.
Testing:
- ran EE tests locally.
Change-Id: I426ad244678dbfe517262dfb7bbf4adc0247a35e
---
M be/src/runtime/descriptors.cc
M be/src/runtime/descriptors.h
M fe/src/main/java/org/apache/impala/analysis/TupleDescriptor.java
M fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java
M testdata/workloads/functional-query/queries/QueryTest/union.test
5 files changed, 51 insertions(+), 8 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/10/6610/7
--
To view, visit http://gerrit.cloudera.org:8080/6610
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I426ad244678dbfe517262dfb7bbf4adc0247a35e
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
[Impala-ASF-CR] IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
......................................................................
Patch Set 6:
Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/506/
--
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: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6610/1/be/src/runtime/descriptors.cc
File be/src/runtime/descriptors.cc:
Line 344: // The vectors get copied because we intend to sort them and it is better to keep this
> Taras and I talked, and the reason why sorting in the FE does not work is t
Can you point out an example of where that assumption is used? This all seems very fragile.
--
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: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Taras Bobrovytsky has posted comments on this change.
Change subject: IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
......................................................................
Patch Set 5:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/6610/5/be/src/runtime/descriptors.h
File be/src/runtime/descriptors.h:
Line 131: int slot_idx() const { return slot_idx_; }
> this doesn't need to be exposed, so let's remove it. TupleDescriptor is alr
Done
Line 426: vector<SlotDescriptor*> SlotsOrderedByIdx() const;
> this shouldn't be pubic either.
Done
--
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: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
......................................................................
Patch Set 7:
FE changes lgtm.
--
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: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Taras Bobrovytsky has posted comments on this change.
Change subject: IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
......................................................................
Patch Set 2:
(2 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->
Done
Line 353: // slots on 'slot_idx'. We define a struct that wraps the comparison function.
> Can remove the last sentence (obvious from code)
Done
--
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 <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Taras Bobrovytsky has uploaded a new patch set (#3).
Change subject: IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
......................................................................
IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
The slot descriptor vectors are not guaranteed to be sorted on the slot
index within a tuple. As a result, TupleDescriptor::LayoutEquals()
sometimes returned a wrong result.
In this patch, we sort the vectors of slot descriptors on the slot index
within the tuple before comparing the vectors.
Testing:
- ran EE tests locally.
Change-Id: I426ad244678dbfe517262dfb7bbf4adc0247a35e
---
M be/src/runtime/descriptors.cc
M be/src/runtime/descriptors.h
M testdata/workloads/functional-query/queries/QueryTest/union.test
3 files changed, 44 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/10/6610/3
--
To view, visit http://gerrit.cloudera.org:8080/6610
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I426ad244678dbfe517262dfb7bbf4adc0247a35e
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
[Impala-ASF-CR] IMPALA-5188: Sort the slots in a tuple before serializing to Thrift
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Taras Bobrovytsky has uploaded a new patch set (#2).
Change subject: IMPALA-5188: Sort the slots in a tuple before serializing to Thrift
......................................................................
IMPALA-5188: Sort the slots in a tuple before serializing to Thrift
The slot descriptor vectors are not guaranteed to be sorted on the slot
index within a tuple. As a result, TupleDescriptor::LayoutEquals()
sometimes returned a wrong result.
In this patch, we sort the slot descriptors in the FE before
serializing to Thrift.
Testing:
- ran EE tests locally.
Change-Id: I426ad244678dbfe517262dfb7bbf4adc0247a35e
---
M be/src/runtime/descriptors.cc
M be/src/runtime/descriptors.h
M fe/src/main/java/org/apache/impala/analysis/DescriptorTable.java
M fe/src/main/java/org/apache/impala/analysis/TupleDescriptor.java
M fe/src/main/java/org/apache/impala/planner/KuduScanNode.java
M testdata/workloads/functional-query/queries/QueryTest/union.test
6 files changed, 33 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/10/6610/2
--
To view, visit http://gerrit.cloudera.org:8080/6610
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I426ad244678dbfe517262dfb7bbf4adc0247a35e
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
[Impala-ASF-CR] IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
......................................................................
Patch Set 6: -Code-Review
> (1 comment)
What's the plan for this? Alex, is that tuple expected?
--
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: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
......................................................................
Patch Set 6:
Build failed: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/515/
--
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: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
......................................................................
Patch Set 7: Code-Review+2
--
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: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
......................................................................
Patch Set 6:
Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/514/
--
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: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Taras Bobrovytsky has uploaded a new patch set (#5).
Change subject: IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
......................................................................
IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
The slot descriptor vectors are not guaranteed to be sorted on the slot
index within a tuple. As a result, TupleDescriptor::LayoutEquals()
sometimes returned a wrong result.
In this patch, we sort the vectors of slot descriptors on the slot index
within the tuple before comparing the vectors.
Testing:
- ran EE tests locally.
Change-Id: I426ad244678dbfe517262dfb7bbf4adc0247a35e
---
M be/src/runtime/descriptors.cc
M be/src/runtime/descriptors.h
M testdata/workloads/functional-query/queries/QueryTest/union.test
3 files changed, 40 insertions(+), 8 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/10/6610/5
--
To view, visit http://gerrit.cloudera.org:8080/6610
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I426ad244678dbfe517262dfb7bbf4adc0247a35e
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
[Impala-ASF-CR] IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Taras Bobrovytsky has posted comments on this change.
Change subject: IMPALA-5188: Add slot sorting in TupleDescriptor::LayoutEquals()
......................................................................
Patch Set 2:
(1 comment)
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
> Question more than comment: The query here is a simplified version of one f
Several queries were already added as part of the union passthrough patch (see above). In my opinion the queries that we already have + the query generator should provide sufficient coverage.
--
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 <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes