[Impala-ASF-CR] IMPALA-4343,IMPALA-4354: qgen: model INSERTs; write INSERTs from query model

[Michael Brown (Code Review)]

Michael Brown has posted comments on this change.

Change subject: IMPALA-4343,IMPALA-4354: qgen: model INSERTs; write INSERTs 
from query model
..


Patch Set 9:

Thanks Taras! Do you mind submitting this for me since you're a committer and 
this doesn't need to go through GVO?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I38e24da78c49e908449b35f0a6276ebe4236ddba
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Brown 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4343,IMPALA-4354: qgen: model INSERTs; write INSERTs from query model

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change.

Change subject: IMPALA-4343,IMPALA-4354: qgen: model INSERTs; write INSERTs 
from query model
..


Patch Set 9: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I38e24da78c49e908449b35f0a6276ebe4236ddba
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Brown 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4343,IMPALA-4354: qgen: model INSERTs; write INSERTs from query model

[Michael Brown (Code Review)]

Michael Brown has posted comments on this change.

Change subject: IMPALA-4343,IMPALA-4354: qgen: model INSERTs; write INSERTs 
from query model
..


Patch Set 9: Code-Review+1

rebase

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I38e24da78c49e908449b35f0a6276ebe4236ddba
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Brown 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4343,IMPALA-4354: qgen: model INSERTs; write INSERTs from query model

[Michael Brown (Code Review)]

Michael Brown has posted comments on this change.

Change subject: IMPALA-4343,IMPALA-4354: qgen: model INSERTs; write INSERTs 
from query model
..


Patch Set 8: Code-Review+1

(1 comment)

Hi Taras, please see patch set 8.

http://gerrit.cloudera.org:8080/#/c/5162/7/tests/comparison/query.py
File tests/comparison/query.py:

PS7, Line 56: xprs = TableExprList([])
> I just tried this:
Done. Thanks for catching that.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I38e24da78c49e908449b35f0a6276ebe4236ddba
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Brown 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4343,IMPALA-4354: qgen: model INSERTs; write INSERTs from query model

[Michael Brown (Code Review)]

Hello Matthew Jacobs,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/5162

to look at the new patch set (#8).

Change subject: IMPALA-4343,IMPALA-4354: qgen: model INSERTs; write INSERTs 
from query model
..

IMPALA-4343,IMPALA-4354: qgen: model INSERTs; write INSERTs from query model

This patch adds support to the random query generator infrastructure to
model and write SQL INSERTs. It does not actually randomly generate
INSERTs at this time (tracked in IMPALA-4353 and umbrella task
IMPALA-3740) but does provide necessary building blocks to do so.

First, it's necessary to model the INSERTs as part of our data model.
This was done by taking the current notion of a Query and making it a
SelectQuery. We also then create an abstract Query containing some of
the more common methods and attributes. We then model an INSERT query,
INSERT clause, and VALUES clause (IMPALA-4343).

Second, it's necessary to test the basics of this data model. It made
sense to go ahead and implement the necessary SqlWriter methods to write
the SQL for these clauses (IMPALA-4354).

I could then use this writer with some existing and new tests that take
a query written into our data model and write the SQL, verifying they're
correct.

For INSERT into Kudu tables, the equivalent PostgreSQL queries need to
use "ON CONFLICT DO NOTHING", so all existing and new query tests verify
they can be written as PostgreSQL as well.

Testing:
- all the query generator tests pass
- I can run Leopard front_end.py and load older query generator reports,
  browse them, and re-run failed queries
- I can run Leopard controller.py to actually do a query generator
  run
- discrepancy_searcher.py --explain-only ran for hundreds of queries.
  There were no problems writing the SELECT queries

Change-Id: I38e24da78c49e908449b35f0a6276ebe4236ddba
---
M tests/comparison/model_translator.py
M tests/comparison/query.py
M tests/comparison/tests/query_object_testdata.py
M tests/comparison/tests/test_query_objects.py
4 files changed, 642 insertions(+), 68 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/62/5162/8
-- 
To view, visit http://gerrit.cloudera.org:8080/5162
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I38e24da78c49e908449b35f0a6276ebe4236ddba
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Brown 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Taras Bobrovytsky 

[Impala-ASF-CR] IMPALA-4343,IMPALA-4354: qgen: model INSERTs; write INSERTs from query model

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change.

Change subject: IMPALA-4343,IMPALA-4354: qgen: model INSERTs; write INSERTs 
from query model
..


Patch Set 7:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5162/7/tests/comparison/query.py
File tests/comparison/query.py:

PS7, Line 56: self.with_clause.table_exprs
> I talked to Taras separately and the concern here is, does the += modify th
I just tried this:

class A(object):

  def __init__(self):
self._x = [1, 2, 3]

  @property
  def x(self):
return self._x

a = A()
lst = a.x
lst.extend([5, 6])
print a.x # prints [1, 2, 3, 5, 6]

So looks like @property does not return a copy.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I38e24da78c49e908449b35f0a6276ebe4236ddba
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Brown 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4343,IMPALA-4354: qgen: model INSERTs; write INSERTs from query model

[Michael Brown (Code Review)]

Michael Brown has posted comments on this change.

Change subject: IMPALA-4343,IMPALA-4354: qgen: model INSERTs; write INSERTs 
from query model
..


Patch Set 7: Code-Review+1

(8 comments)

Thanks for the review. Please see patch set 7.

http://gerrit.cloudera.org:8080/#/c/5162/5/tests/comparison/model_translator.py
File tests/comparison/model_translator.py:

Line 105:   def _write_query(self, query):
> Where does _write_query get used?
Done


PS5, Line 430: mn_lis
> How about changing the order of if and else to get rid of the not:
Done


http://gerrit.cloudera.org:8080/#/c/5162/5/tests/comparison/query.py
File tests/comparison/query.py:

PS5, Line 35: # reference
> It's a good idea to add a comment explaining what each variable means.
Done


Line 40: self.with_clause = None
> I think it's kind of strange to have both @abstractproperty and the impleme
Done


Line 50: abstract as the clauses that do this differ across query types. 
Since all supported
> return self.with_clause.table_exprs if self.with_clause else []
Done


PS5, Line 61: turns
> I think the word query is not the best choice any more. Let's call this Sel
Wontfix, but left an explanation as to why the name should stay the same.


Line 84: self.group_by_clause = None
> why a blank line?
Done


PS5, Line 661: f __deepcopy__(self, memo):
> Can you explain in more detail what this is? (e.g. is it a list? what are t
Done. It's called "column permutation" but buried deep in docs 
http://www.cloudera.com/documentation/enterprise/latest/topics/impala_insert.html.
 I renamed it "column_list" to match the grammar at the top, instead, and that 
gives a clearer picture of what it is anyway.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I38e24da78c49e908449b35f0a6276ebe4236ddba
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Brown 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4343,IMPALA-4354: qgen: model INSERTs; write INSERTs from query model

[Michael Brown (Code Review)]

Michael Brown has posted comments on this change.

Change subject: IMPALA-4343,IMPALA-4354: qgen: model INSERTs; write INSERTs 
from query model
..


Patch Set 6: Code-Review+1

this is just a rebase; next patch set will address comments as needed

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I38e24da78c49e908449b35f0a6276ebe4236ddba
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Brown 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4343,IMPALA-4354: qgen: model INSERTs; write INSERTs from query model

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change.

Change subject: IMPALA-4343,IMPALA-4354: qgen: model INSERTs; write INSERTs 
from query model
..


Patch Set 5:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/5162/5/tests/comparison/model_translator.py
File tests/comparison/model_translator.py:

Line 105:   def _write_query(self, query):
Where does _write_query get used?


PS5, Line 430: is not
How about changing the order of if and else to get rid of the not:
if insert_clause.column_permutations is None:


http://gerrit.cloudera.org:8080/#/c/5162/5/tests/comparison/query.py
File tests/comparison/query.py:

PS5, Line 35: self.parent
It's a good idea to add a comment explaining what each variable means.


Line 40:   def table_exprs(self):
I think it's kind of strange to have both @abstractproperty and the 
implementation. Is this a common pattern?


Line 50: return table_exprs
return self.with_clause.table_exprs if self.with_clause else []


PS5, Line 61: Query
I think the word query is not the best choice any more. Let's call this 
SelectStatement?


Line 84: 
why a blank line?


PS5, Line 661: column_permutations is an optional sequence of Column objects
Can you explain in more detail what this is? (e.g. is it a list? what are the 
element types?)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I38e24da78c49e908449b35f0a6276ebe4236ddba
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Brown 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4343,IMPALA-4354: qgen: model INSERTs; write INSERTs from query model

[Michael Brown (Code Review)]

Michael Brown has posted comments on this change.

Change subject: IMPALA-4343,IMPALA-4354: qgen: model INSERTs; write INSERTs 
from query model
..


Patch Set 4: Code-Review+1

I've simply chosen different names for the objects, and maintained the name 
Query for SELECT statements. We don't need that special pickling anymore. carry 
+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I38e24da78c49e908449b35f0a6276ebe4236ddba
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Brown 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Brown 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4343,IMPALA-4354: qgen: model INSERTs; write INSERTs from query model

[Michael Brown (Code Review)]

Michael Brown has posted comments on this change.

Change subject: IMPALA-4343,IMPALA-4354: qgen: model INSERTs; write INSERTs 
from query model
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5162/1/tests/comparison/leopard/custom_pickle.py
File tests/comparison/leopard/custom_pickle.py:

PS1, Line 8: 
   :   Thanks to 
https://wiki.python.org/moin/UsingPickle/RenamingModules
> I'm testing another patch set that lets us just not use this code at all. I
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I38e24da78c49e908449b35f0a6276ebe4236ddba
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Brown 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Brown 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4343,IMPALA-4354: qgen: model INSERTs; write INSERTs from query model

[Michael Brown (Code Review)]

Hello Matthew Jacobs,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/5162

to look at the new patch set (#4).

Change subject: IMPALA-4343,IMPALA-4354: qgen: model INSERTs; write INSERTs 
from query model
..

IMPALA-4343,IMPALA-4354: qgen: model INSERTs; write INSERTs from query model

This patch adds support to the random query generator infrastructure to
model and write SQL INSERTs. It does not actually randomly generate
INSERTs at this time (tracked in IMPALA-4353 and umbrella task
IMPALA-3740) but does provide necessary building blocks to do so.

First, it's necessary to model the INSERTs as part of our data model.
This was done by taking the current notion of a Query and making it a
SelectQuery. We also then create an abstract Query containing some of
the more common methods and attributes. We then model an INSERT query,
INSERT clause, and VALUES clause (IMPALA-4343).

Second, it's necessary to test the basics of this data model. It made
sense to go ahead and implement the necessary SqlWriter methods to write
the SQL for these clauses (IMPALA-4354).

I could then use this writer with some existing and new tests that take
a query written into our data model and write the SQL, verifying they're
correct.

For INSERT into Kudu tables, the equivalent PostgreSQL queries need to
use "ON CONFLICT DO NOTHING", so all existing and new query tests verify
they can be written as PostgreSQL as well.

Testing:
- all the query generator tests pass
- I can run Leopard front_end.py and load older query generator reports,
  browse them, and re-run failed queries
- I can run Leopard controller.py to actually do a query generator
  run
- discrepancy_searcher.py --explain-only ran for hundreds of queries.
  There were no problems writing the SELECT queries

Change-Id: I38e24da78c49e908449b35f0a6276ebe4236ddba
---
M tests/comparison/model_translator.py
M tests/comparison/query.py
M tests/comparison/tests/query_object_testdata.py
M tests/comparison/tests/test_query_objects.py
4 files changed, 623 insertions(+), 68 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/62/5162/4
-- 
To view, visit http://gerrit.cloudera.org:8080/5162
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I38e24da78c49e908449b35f0a6276ebe4236ddba
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Brown 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Brown 

[Impala-ASF-CR] IMPALA-4343,IMPALA-4354: qgen: model INSERTs; write INSERTs from query model

[Michael Brown (Code Review)]

Hello Matthew Jacobs,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/5162

to look at the new patch set (#3).

Change subject: IMPALA-4343,IMPALA-4354: qgen: model INSERTs; write INSERTs 
from query model
..

IMPALA-4343,IMPALA-4354: qgen: model INSERTs; write INSERTs from query model

This patch adds support to the random query generator infrastructure to
model and write SQL INSERTs. It does not actually randomly generate
INSERTs at this time (tracked in IMPALA-4353 and umbrella task
IMPALA-3740) but does provide necessary building blocks to do so.

First, it's necessary to model the INSERTs as part of our data model.
This was done by taking the current notion of a Query and making it a
SelectQuery. We also then create an abstract Query containing some of
the more common methods and attributes. We then model an INSERT query,
INSERT clause, and VALUES clause (IMPALA-4343).

Second, it's necessary to test the basics of this data model. It made
sense to go ahead and implement the necessary SqlWriter methods to write
the SQL for these clauses (IMPALA-4354).

I could then use this writer with some existing and new tests that take
a query written into our data model and write the SQL, verifying they're
correct.

For INSERT into Kudu tables, the equivalent PostgreSQL queries need to
use "ON CONFLICT DO NOTHING", so all existing and new query tests verify
they can be written as PostgreSQL as well.

Testing:
- all the query generator tests pass
- I can run Leopard front_end.py and load older query generator reports,
  browse them, and re-run failed queries
- I can run Leopard controller.py to actually do a query generator
  run
- discrepancy_searcher.py --explain-only ran for hundreds of queries.
  There were no problems writing the SELECT queries

Change-Id: I38e24da78c49e908449b35f0a6276ebe4236ddba
---
M tests/comparison/model_translator.py
M tests/comparison/query.py
M tests/comparison/query_flattener.py
M tests/comparison/query_generator.py
M tests/comparison/tests/fake_query.py
M tests/comparison/tests/query_object_testdata.py
M tests/comparison/tests/test_query_objects.py
7 files changed, 627 insertions(+), 69 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/62/5162/3
-- 
To view, visit http://gerrit.cloudera.org:8080/5162
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I38e24da78c49e908449b35f0a6276ebe4236ddba
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Brown 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Brown 

[Impala-ASF-CR] IMPALA-4343,IMPALA-4354: qgen: model INSERTs; write INSERTs from query model

[Michael Brown (Code Review)]

Michael Brown has posted comments on this change.

Change subject: IMPALA-4343,IMPALA-4354: qgen: model INSERTs; write INSERTs 
from query model
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5162/1/tests/comparison/leopard/custom_pickle.py
File tests/comparison/leopard/custom_pickle.py:

PS1, Line 8: 
   :   Thanks to 
https://wiki.python.org/moin/UsingPickle/RenamingModules
> If you can't find the license available you'll probably need to check with 
I'm testing another patch set that lets us just not use this code at all. I'll 
mark this done if my testing passes and I update this review to use that patch 
set.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I38e24da78c49e908449b35f0a6276ebe4236ddba
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Brown 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Brown 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4343,IMPALA-4354: qgen: model INSERTs; write INSERTs from query model

[Matthew Jacobs (Code Review)]

Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-4343,IMPALA-4354: qgen: model INSERTs; write INSERTs 
from query model
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5162/1/tests/comparison/leopard/custom_pickle.py
File tests/comparison/leopard/custom_pickle.py:

PS1, Line 8: 
   :   Thanks to 
https://wiki.python.org/moin/UsingPickle/RenamingModules
> How do I go about finding this out? I had previously looked for copyrights 
If you can't find the license available you'll probably need to check with 
lawyers. Jim or Henry can probably point you in the right direction.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I38e24da78c49e908449b35f0a6276ebe4236ddba
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Brown 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Brown 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4343,IMPALA-4354: qgen: model INSERTs; write INSERTs from query model

[Michael Brown (Code Review)]

Michael Brown has posted comments on this change.

Change subject: IMPALA-4343,IMPALA-4354: qgen: model INSERTs; write INSERTs 
from query model
..


Patch Set 2: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I38e24da78c49e908449b35f0a6276ebe4236ddba
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Brown 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Brown 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4343,IMPALA-4354: qgen: model INSERTs; write INSERTs from query model

[Michael Brown (Code Review)]

Michael Brown has posted comments on this change.

Change subject: IMPALA-4343,IMPALA-4354: qgen: model INSERTs; write INSERTs 
from query model
..


Patch Set 1: Code-Review+1

(3 comments)

Thanks for the review. Carry +1

http://gerrit.cloudera.org:8080/#/c/5162/1/tests/comparison/leopard/custom_pickle.py
File tests/comparison/leopard/custom_pickle.py:

> std file header?
Done


PS1, Line 8: 
   :   Thanks to 
https://wiki.python.org/moin/UsingPickle/RenamingModules
> are we sure licensing is OK?
How do I go about finding this out? I had previously looked for copyrights on 
this page, or on code snippets from the wiki generally, and found none. (The 
only copyright I found was for MoinMoin wiki itself, which isn't what I need.)


http://gerrit.cloudera.org:8080/#/c/5162/1/tests/comparison/query.py
File tests/comparison/query.py:

PS1, Line 43: Since all supported
: queries may have a WITH clause, getting table expressions 
from the WITH clause is
: supported here.
> FYI, other DML statements won't support WITH clauses.
Done. We talked offline about this and it makes sense to leave this as-is, the 
reason being that we could eventually support WITH in DELETE and UPDATE, just 
not now. Since UPSERT is "mostly" INSERT, it currently supports WITH already.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I38e24da78c49e908449b35f0a6276ebe4236ddba
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Brown 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Brown 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4343,IMPALA-4354: qgen: model INSERTs; write INSERTs from query model

[Matthew Jacobs (Code Review)]

Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-4343,IMPALA-4354: qgen: model INSERTs; write INSERTs 
from query model
..


Patch Set 1: Code-Review+1

(3 comments)

http://gerrit.cloudera.org:8080/#/c/5162/1/tests/comparison/leopard/custom_pickle.py
File tests/comparison/leopard/custom_pickle.py:

std file header?


PS1, Line 8: 
   :   Thanks to 
https://wiki.python.org/moin/UsingPickle/RenamingModules
are we sure licensing is OK?


http://gerrit.cloudera.org:8080/#/c/5162/1/tests/comparison/query.py
File tests/comparison/query.py:

PS1, Line 43: Since all supported
: queries may have a WITH clause, getting table expressions 
from the WITH clause is
: supported here.
FYI, other DML statements won't support WITH clauses.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I38e24da78c49e908449b35f0a6276ebe4236ddba
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Brown 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4343,IMPALA-4354: qgen: model INSERTs; write INSERTs from query model

[Michael Brown (Code Review)]

Michael Brown has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/5162

Change subject: IMPALA-4343,IMPALA-4354: qgen: model INSERTs; write INSERTs 
from query model
..

IMPALA-4343,IMPALA-4354: qgen: model INSERTs; write INSERTs from query model

This patch adds support to the random query generator infrastructure to
model and write SQL INSERTs. It does not actually randomly generate
INSERTs at this time (tracked in IMPALA-4353 and umbrella task
IMPALA-3740) but does provide necessary building blocks to do so.

First, it's necessary to model the INSERTs as part of our data model.
This was done by taking the current notion of a Query and making it a
SelectQuery. We also then create an abstract Query containing some of
the more common methods and attributes. We then model an INSERT query,
INSERT clause, and VALUES clause (IMPALA-4343).

Second, it's necessary to test the basics of this data model. It made
sense to go ahead and implement the necessary SqlWriter methods to write
the SQL for these clauses (IMPALA-4354).

I could then use this writer with some existing and new tests that take
a query written into our data model and write the SQL, verifying they're
correct.

For INSERT into Kudu tables, the equivalent PostgreSQL queries need to
use "ON CONFLICT DO NOTHING", so all existing and new query tests verify
they can be written as PostgreSQL as well.

When last doing an end to end test of the changes, I encounterd a
problem I hadn't anticipated (but should have): the Leopard framework no
longer had the ability to unpickle query objects, because the name had
changed. I found a solution on the Python wiki here

https://wiki.python.org/moin/UsingPickle/RenamingModules

and adapted it to my needs.

Last, I made some flake8 adjustments in a few files where I also changed
the pickling.

Testing:
- all the query generator tests pass
- I can run Leopard front_end.py and load older query generator reports,
  browse them, and re-run failed queries
- I can run Leopard controller.py to actually do a query generator
  run
- discrepancy_searcher.py --explain-only ran for hundreds of queries.
  There were no problems writing the SELECT queries

Change-Id: I38e24da78c49e908449b35f0a6276ebe4236ddba
---
M tests/comparison/leopard/controller.py
A tests/comparison/leopard/custom_pickle.py
M tests/comparison/leopard/front_end.py
M tests/comparison/leopard/job.py
M tests/comparison/leopard/report.py
M tests/comparison/leopard/schedule_item.py
M tests/comparison/model_translator.py
M tests/comparison/query.py
M tests/comparison/query_flattener.py
M tests/comparison/query_generator.py
M tests/comparison/tests/fake_query.py
M tests/comparison/tests/query_object_testdata.py
M tests/comparison/tests/test_query_objects.py
13 files changed, 711 insertions(+), 112 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/62/5162/1
-- 
To view, visit http://gerrit.cloudera.org:8080/5162
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I38e24da78c49e908449b35f0a6276ebe4236ddba
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Brown