You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by GitBox <gi...@apache.org> on 2021/05/21 19:54:21 UTC
[GitHub] [ignite] alex-plekhanov opened a new pull request #9119: IGNITE-14555 Create table from query result
alex-plekhanov opened a new pull request #9119:
URL: https://github.com/apache/ignite/pull/9119
Thank you for submitting the pull request to the Apache Ignite.
In order to streamline the review of the contribution
we ask you to ensure the following steps have been taken:
### The Contribution Checklist
- [ ] There is a single JIRA ticket related to the pull request.
- [ ] The web-link to the pull request is attached to the JIRA ticket.
- [ ] The JIRA ticket has the _Patch Available_ state.
- [ ] The pull request body describes changes that have been made.
The description explains _WHAT_ and _WHY_ was made instead of _HOW_.
- [ ] The pull request title is treated as the final commit message.
The following pattern must be used: `IGNITE-XXXX Change summary` where `XXXX` - number of JIRA issue.
- [ ] A reviewer has been mentioned through the JIRA comments
(see [the Maintainers list](https://cwiki.apache.org/confluence/display/IGNITE/How+to+Contribute#HowtoContribute-ReviewProcessandMaintainers))
- [ ] The pull request has been checked by the Teamcity Bot and
the `green visa` attached to the JIRA ticket (see [TC.Bot: Check PR](https://mtcga.gridgain.com/prs.html))
### Notes
- [How to Contribute](https://cwiki.apache.org/confluence/display/IGNITE/How+to+Contribute)
- [Coding abbreviation rules](https://cwiki.apache.org/confluence/display/IGNITE/Abbreviation+Rules)
- [Coding Guidelines](https://cwiki.apache.org/confluence/display/IGNITE/Coding+Guidelines)
- [Apache Ignite Teamcity Bot](https://cwiki.apache.org/confluence/display/IGNITE/Apache+Ignite+Teamcity+Bot)
If you need any help, please email dev@ignite.apache.org or ask anу advice on http://asf.slack.com _#ignite_ channel.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] alex-plekhanov commented on a change in pull request #9119: IGNITE-14555 Create table from query result
Posted by GitBox <gi...@apache.org>.
alex-plekhanov commented on a change in pull request #9119:
URL: https://github.com/apache/ignite/pull/9119#discussion_r714510966
##########
File path: modules/calcite/src/main/codegen/includes/parserImpls.ftl
##########
@@ -148,19 +148,29 @@ SqlCreate SqlCreateTable(Span s, boolean replace) :
final SqlIdentifier id;
final SqlNodeList columnList;
final SqlNodeList optionList;
+ final SqlNode query;
}
{
<TABLE>
ifNotExists = IfNotExistsOpt()
id = CompoundIdentifier()
- columnList = TableElementList()
+ (
+ columnList = TableElementList()
+ |
+ { columnList = null; }
+ )
+ (
+ <AS> query = OrderedQueryOrExpr(ExprContext.ACCEPT_QUERY)
+ |
+ { query = null; }
+ )
(
<WITH> { s.add(this); } optionList = CreateTableOptionList()
|
{ optionList = null; }
)
{
Review comment:
New test added
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] alex-plekhanov closed pull request #9119: IGNITE-14555 Create table from query result
Posted by GitBox <gi...@apache.org>.
alex-plekhanov closed pull request #9119:
URL: https://github.com/apache/ignite/pull/9119
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] alex-plekhanov commented on a change in pull request #9119: IGNITE-14555 Create table from query result
Posted by GitBox <gi...@apache.org>.
alex-plekhanov commented on a change in pull request #9119:
URL: https://github.com/apache/ignite/pull/9119#discussion_r717265625
##########
File path: modules/calcite/src/main/codegen/includes/parserImpls.ftl
##########
@@ -142,25 +146,55 @@ SqlNodeList TableElementList() :
}
}
+SqlNodeList TableSimpleColumnList() :
+{
+ final Span s;
+ final List<SqlNode> list = new ArrayList<SqlNode>();
+ SqlIdentifier id;
+}
+{
+ <LPAREN> { s = span(); }
+ id = SimpleIdentifier() {
+ list.add(SqlDdlNodes.column(s.add(id).end(this), id, null, null, null));
Review comment:
Ok, fixed
##########
File path: modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/TableDdlIntegrationTest.java
##########
@@ -275,6 +275,97 @@ public void createTableOnExistingCache() {
assertEquals(2, cache.size(CachePeekMode.PRIMARY));
}
+ /**
+ * Creates table as select.
+ */
+ @Test
+ public void createTableAsSelectSimpleCase() {
+ executeSql("create table my_table as select 1 as i, 'test' as s");
+ List<List<?>> res = executeSql("select i, s from my_table");
+
+ assertEquals(1, res.size());
+
+ List<?> row = res.get(0);
+
+ assertEquals(1, row.get(0));
+ assertEquals("test", row.get(1));
+ }
+
+ /**
+ * Creates table with specified columns as select.
+ */
+ @Test
+ public void createTableAsSelectWithColumns() {
Review comment:
Check for column names added to this test
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] alex-plekhanov commented on pull request #9119: IGNITE-14555 Create table from query result
Posted by GitBox <gi...@apache.org>.
alex-plekhanov commented on pull request #9119:
URL: https://github.com/apache/ignite/pull/9119#issuecomment-928947627
Merged to sql-calcite branch
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] alex-plekhanov commented on a change in pull request #9119: IGNITE-14555 Create table from query result
Posted by GitBox <gi...@apache.org>.
alex-plekhanov commented on a change in pull request #9119:
URL: https://github.com/apache/ignite/pull/9119#discussion_r717265778
##########
File path: modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/TableDdlIntegrationTest.java
##########
@@ -275,6 +275,97 @@ public void createTableOnExistingCache() {
assertEquals(2, cache.size(CachePeekMode.PRIMARY));
}
+ /**
+ * Creates table as select.
+ */
+ @Test
+ public void createTableAsSelectSimpleCase() {
+ executeSql("create table my_table as select 1 as i, 'test' as s");
+ List<List<?>> res = executeSql("select i, s from my_table");
+
+ assertEquals(1, res.size());
+
+ List<?> row = res.get(0);
+
+ assertEquals(1, row.get(0));
+ assertEquals("test", row.get(1));
+ }
+
+ /**
+ * Creates table with specified columns as select.
+ */
+ @Test
+ public void createTableAsSelectWithColumns() {
Review comment:
Check for column names added to this test
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] alex-plekhanov commented on a change in pull request #9119: IGNITE-14555 Create table from query result
Posted by GitBox <gi...@apache.org>.
alex-plekhanov commented on a change in pull request #9119:
URL: https://github.com/apache/ignite/pull/9119#discussion_r714551337
##########
File path: modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/PlannerHelper.java
##########
@@ -107,6 +121,72 @@ public static IgniteRel optimize(SqlNode sqlNode, IgnitePlanner planner, IgniteL
}
}
+ /**
+ * Creates physical plan for "INSERT INTO table SELECT ..." based on "CREATE TABLE table AS SELECT ..." statement.
+ */
+ public static IgniteRel makeCreateTableAsSelectPlan(CreateTableCommand createTableCmd, PlanningContext ctx,
Review comment:
> Have you considered an implicit rewriting of query with AS clause to two distinct statements (CREATE + INSERT)?
I've changed it to CREATE+INSERT, but now there is yet another planning phase on the execution phase for CREATE AS SELECT statement. Changes ready for review, please have a look again.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] korlov42 commented on a change in pull request #9119: IGNITE-14555 Create table from query result
Posted by GitBox <gi...@apache.org>.
korlov42 commented on a change in pull request #9119:
URL: https://github.com/apache/ignite/pull/9119#discussion_r713153348
##########
File path: modules/calcite/src/main/codegen/includes/parserImpls.ftl
##########
@@ -148,19 +148,29 @@ SqlCreate SqlCreateTable(Span s, boolean replace) :
final SqlIdentifier id;
final SqlNodeList columnList;
final SqlNodeList optionList;
+ final SqlNode query;
}
{
<TABLE>
ifNotExists = IfNotExistsOpt()
id = CompoundIdentifier()
- columnList = TableElementList()
+ (
+ columnList = TableElementList()
Review comment:
AFAIK column list in case of `CREATE TABLE ... AS` should contain only aliases without any types specs or constraints
##########
File path: modules/calcite/src/main/codegen/includes/parserImpls.ftl
##########
@@ -148,19 +148,29 @@ SqlCreate SqlCreateTable(Span s, boolean replace) :
final SqlIdentifier id;
final SqlNodeList columnList;
final SqlNodeList optionList;
+ final SqlNode query;
}
{
<TABLE>
ifNotExists = IfNotExistsOpt()
id = CompoundIdentifier()
- columnList = TableElementList()
+ (
+ columnList = TableElementList()
+ |
+ { columnList = null; }
+ )
+ (
+ <AS> query = OrderedQueryOrExpr(ExprContext.ACCEPT_QUERY)
Review comment:
it's better to have `AS` part as a last one to not to mess to which part of a statement a `WITH` clause belongs to. For example:
```
1) CREATE TABLE my_t AS SELECT a, b FROM another_T WITH backups=3
2) CREATE TABLE my_t WITH backups=3 AS SELECT a, b FROM another_T
```
The second option seems better to me.
##########
File path: modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/PlannerHelper.java
##########
@@ -107,6 +121,72 @@ public static IgniteRel optimize(SqlNode sqlNode, IgnitePlanner planner, IgniteL
}
}
+ /**
+ * Creates physical plan for "INSERT INTO table SELECT ..." based on "CREATE TABLE table AS SELECT ..." statement.
+ */
+ public static IgniteRel makeCreateTableAsSelectPlan(CreateTableCommand createTableCmd, PlanningContext ctx,
Review comment:
Honestly speaking, I don't like the idea to create execution plan by hand. First, the table internals has leaked to the the planner level (row type format to feed to ModifyNode). And second, in such case we need to reimplement here all optimizations which planner would possibly apply. For example like this one https://issues.apache.org/jira/browse/IGNITE-12692.
Have you considered an implicit rewriting of query with AS clause to two distinct statements (`CREATE` + `INSERT`)?
##########
File path: modules/calcite/src/main/codegen/includes/parserImpls.ftl
##########
@@ -148,19 +148,29 @@ SqlCreate SqlCreateTable(Span s, boolean replace) :
final SqlIdentifier id;
final SqlNodeList columnList;
final SqlNodeList optionList;
+ final SqlNode query;
}
{
<TABLE>
ifNotExists = IfNotExistsOpt()
id = CompoundIdentifier()
- columnList = TableElementList()
+ (
+ columnList = TableElementList()
+ |
+ { columnList = null; }
+ )
+ (
+ <AS> query = OrderedQueryOrExpr(ExprContext.ACCEPT_QUERY)
+ |
+ { query = null; }
+ )
(
<WITH> { s.add(this); } optionList = CreateTableOptionList()
|
{ optionList = null; }
)
{
Review comment:
Please extend `org.apache.ignite.internal.processors.query.calcite.sql.SqlDdlParserTest` with tests to verify the new clause format
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] korlov42 commented on a change in pull request #9119: IGNITE-14555 Create table from query result
Posted by GitBox <gi...@apache.org>.
korlov42 commented on a change in pull request #9119:
URL: https://github.com/apache/ignite/pull/9119#discussion_r715436866
##########
File path: modules/calcite/src/main/codegen/includes/parserImpls.ftl
##########
@@ -142,25 +146,55 @@ SqlNodeList TableElementList() :
}
}
+SqlNodeList TableSimpleColumnList() :
+{
+ final Span s;
+ final List<SqlNode> list = new ArrayList<SqlNode>();
+ SqlIdentifier id;
+}
+{
+ <LPAREN> { s = span(); }
+ id = SimpleIdentifier() {
+ list.add(SqlDdlNodes.column(s.add(id).end(this), id, null, null, null));
Review comment:
why don't just use SqlIdentifier itself? It will allow to avoid unnecessary branching in `IgniteSqlCreateTable#unparse`
##########
File path: modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/TableDdlIntegrationTest.java
##########
@@ -275,6 +275,97 @@ public void createTableOnExistingCache() {
assertEquals(2, cache.size(CachePeekMode.PRIMARY));
}
+ /**
+ * Creates table as select.
+ */
+ @Test
+ public void createTableAsSelectSimpleCase() {
+ executeSql("create table my_table as select 1 as i, 'test' as s");
+ List<List<?>> res = executeSql("select i, s from my_table");
+
+ assertEquals(1, res.size());
+
+ List<?> row = res.get(0);
+
+ assertEquals(1, row.get(0));
+ assertEquals("test", row.get(1));
+ }
+
+ /**
+ * Creates table with specified columns as select.
+ */
+ @Test
+ public void createTableAsSelectWithColumns() {
Review comment:
let's add a few test to verify column names of just created tables
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] alex-plekhanov commented on pull request #9119: IGNITE-14555 Create table from query result
Posted by GitBox <gi...@apache.org>.
alex-plekhanov commented on pull request #9119:
URL: https://github.com/apache/ignite/pull/9119#issuecomment-928947627
Merged to sql-calcite branch
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] alex-plekhanov commented on a change in pull request #9119: IGNITE-14555 Create table from query result
Posted by GitBox <gi...@apache.org>.
alex-plekhanov commented on a change in pull request #9119:
URL: https://github.com/apache/ignite/pull/9119#discussion_r714510841
##########
File path: modules/calcite/src/main/codegen/includes/parserImpls.ftl
##########
@@ -148,19 +148,29 @@ SqlCreate SqlCreateTable(Span s, boolean replace) :
final SqlIdentifier id;
final SqlNodeList columnList;
final SqlNodeList optionList;
+ final SqlNode query;
}
{
<TABLE>
ifNotExists = IfNotExistsOpt()
id = CompoundIdentifier()
- columnList = TableElementList()
+ (
+ columnList = TableElementList()
+ |
+ { columnList = null; }
+ )
+ (
+ <AS> query = OrderedQueryOrExpr(ExprContext.ACCEPT_QUERY)
Review comment:
Fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] alex-plekhanov commented on a change in pull request #9119: IGNITE-14555 Create table from query result
Posted by GitBox <gi...@apache.org>.
alex-plekhanov commented on a change in pull request #9119:
URL: https://github.com/apache/ignite/pull/9119#discussion_r714510733
##########
File path: modules/calcite/src/main/codegen/includes/parserImpls.ftl
##########
@@ -148,19 +148,29 @@ SqlCreate SqlCreateTable(Span s, boolean replace) :
final SqlIdentifier id;
final SqlNodeList columnList;
final SqlNodeList optionList;
+ final SqlNode query;
}
{
<TABLE>
ifNotExists = IfNotExistsOpt()
id = CompoundIdentifier()
- columnList = TableElementList()
+ (
+ columnList = TableElementList()
Review comment:
You are right, thanks, fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] alex-plekhanov closed pull request #9119: IGNITE-14555 Create table from query result
Posted by GitBox <gi...@apache.org>.
alex-plekhanov closed pull request #9119:
URL: https://github.com/apache/ignite/pull/9119
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] alex-plekhanov commented on a change in pull request #9119: IGNITE-14555 Create table from query result
Posted by GitBox <gi...@apache.org>.
alex-plekhanov commented on a change in pull request #9119:
URL: https://github.com/apache/ignite/pull/9119#discussion_r717265625
##########
File path: modules/calcite/src/main/codegen/includes/parserImpls.ftl
##########
@@ -142,25 +146,55 @@ SqlNodeList TableElementList() :
}
}
+SqlNodeList TableSimpleColumnList() :
+{
+ final Span s;
+ final List<SqlNode> list = new ArrayList<SqlNode>();
+ SqlIdentifier id;
+}
+{
+ <LPAREN> { s = span(); }
+ id = SimpleIdentifier() {
+ list.add(SqlDdlNodes.column(s.add(id).end(this), id, null, null, null));
Review comment:
Ok, fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org