You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Matthew Jacobs (Code Review)" <ge...@cloudera.org> on 2017/08/03 17:17:04 UTC
[Impala-ASF-CR] IMPALA-5757: Order-dependent comparison fails in test kudu
Matthew Jacobs has uploaded a new change for review.
http://gerrit.cloudera.org:8080/7575
Change subject: IMPALA-5757: Order-dependent comparison fails in test_kudu
......................................................................
IMPALA-5757: Order-dependent comparison fails in test_kudu
On Ubuntu 16.04, it seems that table properties may be
returned in a different order from that expected by
TestShowCreateTable in test_kudu.py.
There is only one affected test case, because all other test
cases only have a single table property.
This fixes the affected test by checking for both table
property strings in the show create table result, rather
than validating the result matches an expected string
exactly.
Change-Id: I64ce0c49fcbab3a4810d52c131bc176f8567ff9e
---
M tests/query_test/test_kudu.py
1 file changed, 26 insertions(+), 20 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/75/7575/1
--
To view, visit http://gerrit.cloudera.org:8080/7575
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I64ce0c49fcbab3a4810d52c131bc176f8567ff9e
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
[Impala-ASF-CR] IMPALA-5757: Order-dependent comparison fails in test kudu
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change.
Change subject: IMPALA-5757: Order-dependent comparison fails in test_kudu
......................................................................
Patch Set 1:
> (1 comment)
I recently changed several places in the FE to use LinkedHashMaps in order to get deterministic ordering. That included ToSqlUtils.java:212, so I'm surprised the order is not deterministic. I suspect that file has the code that needs fixing, possibly L427 propertyMapToSql(). Maybe it'd be best to sort the properties to be completely independent from the underlying storage.
--
To view, visit http://gerrit.cloudera.org:8080/7575
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I64ce0c49fcbab3a4810d52c131bc176f8567ff9e
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5757: Order-dependent comparison fails in test kudu
Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has posted comments on this change.
Change subject: IMPALA-5757: Order-dependent comparison fails in test_kudu
......................................................................
Patch Set 1:
> > (1 comment)
>
> I recently changed several places in the FE to use LinkedHashMaps
> in order to get deterministic ordering. That included
> ToSqlUtils.java:212, so I'm surprised the order is not
> deterministic. I suspect that file has the code that needs fixing,
> possibly L427 propertyMapToSql(). Maybe it'd be best to sort the
> properties to be completely independent from the underlying
> storage.
Yup, that looks correct. I think we can just create a LinkedHashSet from the entrySet(). Hopefully that won't break too many existing tests.
--
To view, visit http://gerrit.cloudera.org:8080/7575
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I64ce0c49fcbab3a4810d52c131bc176f8567ff9e
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5757: Order-dependent comparison fails in test kudu
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: IMPALA-5757: Order-dependent comparison fails in test_kudu
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7575/1//COMMIT_MSG
Commit Message:
PS1, Line 16: checking for both
Could we change the table property printing to be deterministic, rather than changing the test?
--
To view, visit http://gerrit.cloudera.org:8080/7575
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I64ce0c49fcbab3a4810d52c131bc176f8567ff9e
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5757: Order-dependent comparison fails in test kudu
Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has posted comments on this change.
Change subject: IMPALA-5757: Order-dependent comparison fails in test_kudu
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7575/1//COMMIT_MSG
Commit Message:
PS1, Line 16: checking for both
> Could we change the table property printing to be deterministic, rather tha
Yeah good point. I assumed it'd be a big change as it could reorder existing tests, but I didn't validate that. I'll check this afternoon.
--
To view, visit http://gerrit.cloudera.org:8080/7575
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I64ce0c49fcbab3a4810d52c131bc176f8567ff9e
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5757: Order-dependent comparison fails in test kudu
Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has abandoned this change.
Change subject: IMPALA-5757: Order-dependent comparison fails in test_kudu
......................................................................
Abandoned
created a new commit, forgot to take the old Change-Id. New review: https://gerrit.cloudera.org/#/c/7580/
--
To view, visit http://gerrit.cloudera.org:8080/7575
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: abandon
Gerrit-Change-Id: I64ce0c49fcbab3a4810d52c131bc176f8567ff9e
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-5757: Order-dependent comparison fails in test kudu
Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has posted comments on this change.
Change subject: IMPALA-5757: Order-dependent comparison fails in test_kudu
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7575/1//COMMIT_MSG
Commit Message:
PS1, Line 16: checking for both
> I think Lars already did this for some table properties recently.
How soon do we need a fix? If this is blocking you or other engineers, we may need to take this and we can do the more general fix later. I don't have bandwidth to make the bigger change this week.
--
To view, visit http://gerrit.cloudera.org:8080/7575
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I64ce0c49fcbab3a4810d52c131bc176f8567ff9e
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5757: Order-dependent comparison fails in test kudu
Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has posted comments on this change.
Change subject: IMPALA-5757: Order-dependent comparison fails in test_kudu
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7575/1//COMMIT_MSG
Commit Message:
PS1, Line 16: checking for both
> How soon do we need a fix? If this is blocking you or other engineers, we m
I guess you didn't mark this as a blocker so I'll take it to mean this isn't needed immediately. I'll do the more general fix when I have a bit more time.
--
To view, visit http://gerrit.cloudera.org:8080/7575
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I64ce0c49fcbab3a4810d52c131bc176f8567ff9e
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5757: Order-dependent comparison fails in test kudu
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change.
Change subject: IMPALA-5757: Order-dependent comparison fails in test_kudu
......................................................................
Patch Set 1:
> > > (1 comment)
> >
> > I recently changed several places in the FE to use LinkedHashMaps
> > in order to get deterministic ordering. That included
> > ToSqlUtils.java:212, so I'm surprised the order is not
> > deterministic. I suspect that file has the code that needs
> fixing,
> > possibly L427 propertyMapToSql(). Maybe it'd be best to sort the
> > properties to be completely independent from the underlying
> > storage.
>
> Yup, that looks correct. I think we can just create a LinkedHashSet
> from the entrySet(). Hopefully that won't break too many existing
> tests.
At that poing propertyMap is already a LinkedHashMap, so I think the order of the entrySet should be stable (see https://stackoverflow.com/questions/1190083/does-entryset-in-a-linkedhashmap-also-guarantee-order).
I think msTable.getParameters() returns a Map<>, which means we may have to sort it to get a deterministic order.
--
To view, visit http://gerrit.cloudera.org:8080/7575
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I64ce0c49fcbab3a4810d52c131bc176f8567ff9e
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5757: Order-dependent comparison fails in test kudu
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-5757: Order-dependent comparison fails in test_kudu
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7575/1//COMMIT_MSG
Commit Message:
PS1, Line 16: checking for both
> Yeah good point. I assumed it'd be a big change as it could reorder existin
I think Lars already did this for some table properties recently.
--
To view, visit http://gerrit.cloudera.org:8080/7575
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I64ce0c49fcbab3a4810d52c131bc176f8567ff9e
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5757: Order-dependent comparison fails in test kudu
Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has posted comments on this change.
Change subject: IMPALA-5757: Order-dependent comparison fails in test_kudu
......................................................................
Patch Set 1:
> > > > (1 comment)
> > >
> > > I recently changed several places in the FE to use
> LinkedHashMaps
> > > in order to get deterministic ordering. That included
> > > ToSqlUtils.java:212, so I'm surprised the order is not
> > > deterministic. I suspect that file has the code that needs
> > fixing,
> > > possibly L427 propertyMapToSql(). Maybe it'd be best to sort
> the
> > > properties to be completely independent from the underlying
> > > storage.
> >
> > Yup, that looks correct. I think we can just create a
> LinkedHashSet
> > from the entrySet(). Hopefully that won't break too many existing
> > tests.
>
> At that poing propertyMap is already a LinkedHashMap, so I think
> the order of the entrySet should be stable (see https://stackoverflow.com/questions/1190083/does-entryset-in-a-linkedhashmap-also-guarantee-order).
>
> I think msTable.getParameters() returns a Map<>, which means we may
> have to sort it to get a deterministic order.
Yeah I was just realizing that as well. Thanks
--
To view, visit http://gerrit.cloudera.org:8080/7575
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I64ce0c49fcbab3a4810d52c131bc176f8567ff9e
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No