You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by "snuyanzin (via GitHub)" <gi...@apache.org> on 2024/09/11 23:44:34 UTC
[PR] [FLINK-36266][table] Insert into as select * behaves incorrect [flink]
snuyanzin opened a new pull request, #25316:
URL: https://github.com/apache/flink/pull/25316
## What is the purpose of the change
The PR fixes behavior for insert into queries with select * like
```sql
INSERT INTO t(f0, f1 ,f2) SELECT * FROM t1;
```
## Verifying this change
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/calcite/FlinkCalciteSqlValidatorTest.java
## Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): ( no)
- The public API, i.e., is any changed class annotated with `@Public(Evolving)`: ( no)
- The serializers: ( no)
- The runtime per-record code paths (performance sensitive): ( no)
- Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: ( no)
- The S3 file system connector: ( no)
## Documentation
- Does this pull request introduce a new feature? ( no)
- If yes, how is the feature documented? (not applicable )
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-36266][table] Insert into as select * behaves incorrect [flink]
Posted by "jnh5y (via GitHub)" <gi...@apache.org>.
jnh5y commented on code in PR #25316:
URL: https://github.com/apache/flink/pull/25316#discussion_r1829793928
##########
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/calcite/FlinkCalciteSqlValidatorTest.java:
##########
@@ -50,74 +67,43 @@ void testUpsertInto() {
"UPSERT INTO statement is not supported. Please use INSERT INTO instead.");
}
- @Test
- void testInsertIntoShouldColumnMismatchWithValues() {
- assertThatThrownBy(() -> plannerMocks.getParser().parse("INSERT INTO t2 (a,b) VALUES(1)"))
- .isInstanceOf(ValidationException.class)
- .hasMessageContaining(" Number of columns must match number of query columns");
- }
-
- @Test
- void testInsertIntoShouldColumnMismatchWithSelect() {
- assertThatThrownBy(() -> plannerMocks.getParser().parse("INSERT INTO t2 (a,b) SELECT 1"))
- .isInstanceOf(ValidationException.class)
- .hasMessageContaining(" Number of columns must match number of query columns");
- }
-
- @Test
- void testInsertIntoShouldColumnMismatchWithLastValue() {
- assertThatThrownBy(
- () ->
- plannerMocks
- .getParser()
- .parse("INSERT INTO t2 (a,b) VALUES (1,2), (3)"))
- .isInstanceOf(ValidationException.class)
- .hasMessageContaining(" Number of columns must match number of query columns");
- }
-
- @Test
- void testInsertIntoShouldColumnMismatchWithFirstValue() {
- assertThatThrownBy(
- () ->
- plannerMocks
- .getParser()
- .parse("INSERT INTO t2 (a,b) VALUES (1), (2,3)"))
- .isInstanceOf(ValidationException.class)
- .hasMessageContaining(" Number of columns must match number of query columns");
- }
-
- @Test
- void testInsertIntoShouldColumnMismatchWithMultiFieldValues() {
- assertThatThrownBy(
- () ->
- plannerMocks
- .getParser()
- .parse("INSERT INTO t2 (a,b) VALUES (1,2), (3,4,5)"))
+ @ParameterizedTest
+ @ValueSource(
+ strings = {
+ "INSERT INTO t2 (a, b) VALUES(1)",
+ "INSERT INTO t2 (a, b) VALUES (1, 2), (3)",
+ "INSERT INTO t2 (a, b) VALUES (1), (2, 3)",
+ "INSERT INTO t2 (a, b) VALUES (1, 2), (3, 4, 5)",
+ "INSERT INTO t2 (a, b) SELECT 1",
+ "INSERT INTO t2 (a, b) SELECT * FROM t1",
+ "INSERT INTO t2 (a, b) SELECT *, 42 FROM t2_copy",
+ "INSERT INTO t2 (a, b) SELECT 42, * FROM t2_copy",
+ "INSERT INTO t2 (a, b) SELECT * FROM t_nested",
+ "INSERT INTO t2 (a, b) TABLE t_nested",
+ "INSERT INTO t2 (a, b) SELECT * FROM (TABLE t_nested)"
+ })
+ void testInvalidNumberOfColumnsWhileInsertInto(String sql) {
+ assertThatThrownBy(() -> plannerMocks.getParser().parse(sql))
.isInstanceOf(ValidationException.class)
.hasMessageContaining(" Number of columns must match number of query columns");
}
- @Test
- void testInsertIntoShouldNotColumnMismatchWithValues() {
- assertDoesNotThrow(
- () -> {
- plannerMocks.getParser().parse("INSERT INTO t2 (a,b) VALUES (1,2), (3,4)");
- });
- }
-
- @Test
- void testInsertIntoShouldNotColumnMismatchWithSelect() {
- assertDoesNotThrow(
- () -> {
- plannerMocks.getParser().parse("INSERT INTO t2 (a,b) Select 1, 2");
- });
- }
-
- @Test
- void testInsertIntoShouldNotColumnMismatchWithSingleColValues() {
+ @ParameterizedTest
+ @ValueSource(
+ strings = {
+ "INSERT INTO t2 (a, b) VALUES (1, 2), (3, 4)",
+ "INSERT INTO t2 (a) VALUES (1), (3)",
+ "INSERT INTO t2 (a, b) SELECT 1, 2",
+ "INSERT INTO t2 (a, b) SELECT * FROM t2_copy",
+ "INSERT INTO t2 (a, b) SELECT *, 42 FROM t1",
+ "INSERT INTO t2 (a, b) SELECT 42, * FROM t1",
+ "INSERT INTO t2 (a, b) SELECT f.* FROM t_nested",
+ "INSERT INTO t2 (a, b) TABLE t2_copy"
Review Comment:
Ok, so `identifiersSize == 0` means that all members of `getSelectList` is a STAR, right?
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-36266][table] Insert into as select * behaves incorrect [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on PR #25316:
URL: https://github.com/apache/flink/pull/25316#issuecomment-2438463937
@flinkbot run azure
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-36266][table] Insert into as select * behaves incorrect [flink]
Posted by "jnh5y (via GitHub)" <gi...@apache.org>.
jnh5y commented on PR #25316:
URL: https://github.com/apache/flink/pull/25316#issuecomment-2457624570
Would it make sense to add a test case to something like `TableSinkITCase` to validate things end-to-end and also prevent regressions?
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-36266][table] Insert into as select * behaves incorrect [flink]
Posted by "jnh5y (via GitHub)" <gi...@apache.org>.
jnh5y commented on code in PR #25316:
URL: https://github.com/apache/flink/pull/25316#discussion_r1829826534
##########
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/calcite/FlinkCalciteSqlValidatorTest.java:
##########
@@ -50,74 +67,43 @@ void testUpsertInto() {
"UPSERT INTO statement is not supported. Please use INSERT INTO instead.");
}
- @Test
- void testInsertIntoShouldColumnMismatchWithValues() {
- assertThatThrownBy(() -> plannerMocks.getParser().parse("INSERT INTO t2 (a,b) VALUES(1)"))
- .isInstanceOf(ValidationException.class)
- .hasMessageContaining(" Number of columns must match number of query columns");
- }
-
- @Test
- void testInsertIntoShouldColumnMismatchWithSelect() {
- assertThatThrownBy(() -> plannerMocks.getParser().parse("INSERT INTO t2 (a,b) SELECT 1"))
- .isInstanceOf(ValidationException.class)
- .hasMessageContaining(" Number of columns must match number of query columns");
- }
-
- @Test
- void testInsertIntoShouldColumnMismatchWithLastValue() {
- assertThatThrownBy(
- () ->
- plannerMocks
- .getParser()
- .parse("INSERT INTO t2 (a,b) VALUES (1,2), (3)"))
- .isInstanceOf(ValidationException.class)
- .hasMessageContaining(" Number of columns must match number of query columns");
- }
-
- @Test
- void testInsertIntoShouldColumnMismatchWithFirstValue() {
- assertThatThrownBy(
- () ->
- plannerMocks
- .getParser()
- .parse("INSERT INTO t2 (a,b) VALUES (1), (2,3)"))
- .isInstanceOf(ValidationException.class)
- .hasMessageContaining(" Number of columns must match number of query columns");
- }
-
- @Test
- void testInsertIntoShouldColumnMismatchWithMultiFieldValues() {
- assertThatThrownBy(
- () ->
- plannerMocks
- .getParser()
- .parse("INSERT INTO t2 (a,b) VALUES (1,2), (3,4,5)"))
+ @ParameterizedTest
+ @ValueSource(
+ strings = {
+ "INSERT INTO t2 (a, b) VALUES(1)",
+ "INSERT INTO t2 (a, b) VALUES (1, 2), (3)",
+ "INSERT INTO t2 (a, b) VALUES (1), (2, 3)",
+ "INSERT INTO t2 (a, b) VALUES (1, 2), (3, 4, 5)",
+ "INSERT INTO t2 (a, b) SELECT 1",
+ "INSERT INTO t2 (a, b) SELECT * FROM t1",
+ "INSERT INTO t2 (a, b) SELECT *, 42 FROM t2_copy",
+ "INSERT INTO t2 (a, b) SELECT 42, * FROM t2_copy",
+ "INSERT INTO t2 (a, b) SELECT * FROM t_nested",
+ "INSERT INTO t2 (a, b) TABLE t_nested",
+ "INSERT INTO t2 (a, b) SELECT * FROM (TABLE t_nested)"
+ })
+ void testInvalidNumberOfColumnsWhileInsertInto(String sql) {
+ assertThatThrownBy(() -> plannerMocks.getParser().parse(sql))
.isInstanceOf(ValidationException.class)
.hasMessageContaining(" Number of columns must match number of query columns");
}
- @Test
- void testInsertIntoShouldNotColumnMismatchWithValues() {
- assertDoesNotThrow(
- () -> {
- plannerMocks.getParser().parse("INSERT INTO t2 (a,b) VALUES (1,2), (3,4)");
- });
- }
-
- @Test
- void testInsertIntoShouldNotColumnMismatchWithSelect() {
- assertDoesNotThrow(
- () -> {
- plannerMocks.getParser().parse("INSERT INTO t2 (a,b) Select 1, 2");
- });
- }
-
- @Test
- void testInsertIntoShouldNotColumnMismatchWithSingleColValues() {
+ @ParameterizedTest
+ @ValueSource(
+ strings = {
+ "INSERT INTO t2 (a, b) VALUES (1, 2), (3, 4)",
+ "INSERT INTO t2 (a) VALUES (1), (3)",
+ "INSERT INTO t2 (a, b) SELECT 1, 2",
+ "INSERT INTO t2 (a, b) SELECT * FROM t2_copy",
+ "INSERT INTO t2 (a, b) SELECT *, 42 FROM t1",
+ "INSERT INTO t2 (a, b) SELECT 42, * FROM t1",
+ "INSERT INTO t2 (a, b) SELECT f.* FROM t_nested",
+ "INSERT INTO t2 (a, b) TABLE t2_copy"
Review Comment:
I like the change to boolean.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-36266][table] Insert into as select * behaves incorrect [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on PR #25316:
URL: https://github.com/apache/flink/pull/25316#issuecomment-2439866522
@flinkbot run azure
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-36266][table] Insert into as select * behaves incorrect [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #25316:
URL: https://github.com/apache/flink/pull/25316#discussion_r1829780691
##########
flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/calcite/SqlRewriterUtils.scala:
##########
@@ -130,8 +131,11 @@ object SqlRewriterUtils {
call.getKind match {
case SqlKind.SELECT =>
val sqlSelect = call.asInstanceOf[SqlSelect]
-
- if (targetPosition.nonEmpty && sqlSelect.getSelectList.size() != targetPosition.size()) {
+ val identifiersSize = sqlSelect.getSelectList.count(s => s.isInstanceOf[SqlIdentifier])
+ if (
+ identifiersSize == 0
Review Comment:
continued discussion in https://github.com/apache/flink/pull/25316#discussion_r1829780200
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-36266][table] Insert into as select * behaves incorrect [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #25316:
URL: https://github.com/apache/flink/pull/25316#discussion_r1829790186
##########
flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/calcite/SqlRewriterUtils.scala:
##########
@@ -157,9 +161,14 @@ object SqlRewriterUtils {
operands.get(1).asInstanceOf[SqlNodeList],
operands.get(2),
operands.get(3))
+ case SqlKind.EXPLICIT_TABLE =>
+ val operands = call.getOperandList
+ val expTable = new ExplicitTableSqlSelect(
+ operands.get(0).asInstanceOf[SqlIdentifier],
+ Collections.emptyList())
+ rewriterUtils.rewriteSelect(expTable, targetRowType, assignedFields, targetPosition)
// Not support:
// case SqlKind.WITH =>
Review Comment:
~~looks like for that first should be supported in Calcite~~
there is support in Oracle's style like
```sql
INSERT INTO ...
WITH cte ...
SELECT ...
```
added support for that
initially I was looking for Postgres style like
```sql
WITH cte ...
INSERT INTO ...
SELECT ...
```
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-36266][table] Insert into as select * behaves incorrect [flink]
Posted by "jnh5y (via GitHub)" <gi...@apache.org>.
jnh5y commented on code in PR #25316:
URL: https://github.com/apache/flink/pull/25316#discussion_r1829716133
##########
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/calcite/FlinkCalciteSqlValidatorTest.java:
##########
@@ -50,74 +67,43 @@ void testUpsertInto() {
"UPSERT INTO statement is not supported. Please use INSERT INTO instead.");
}
- @Test
- void testInsertIntoShouldColumnMismatchWithValues() {
- assertThatThrownBy(() -> plannerMocks.getParser().parse("INSERT INTO t2 (a,b) VALUES(1)"))
- .isInstanceOf(ValidationException.class)
- .hasMessageContaining(" Number of columns must match number of query columns");
- }
-
- @Test
- void testInsertIntoShouldColumnMismatchWithSelect() {
- assertThatThrownBy(() -> plannerMocks.getParser().parse("INSERT INTO t2 (a,b) SELECT 1"))
- .isInstanceOf(ValidationException.class)
- .hasMessageContaining(" Number of columns must match number of query columns");
- }
-
- @Test
- void testInsertIntoShouldColumnMismatchWithLastValue() {
- assertThatThrownBy(
- () ->
- plannerMocks
- .getParser()
- .parse("INSERT INTO t2 (a,b) VALUES (1,2), (3)"))
- .isInstanceOf(ValidationException.class)
- .hasMessageContaining(" Number of columns must match number of query columns");
- }
-
- @Test
- void testInsertIntoShouldColumnMismatchWithFirstValue() {
- assertThatThrownBy(
- () ->
- plannerMocks
- .getParser()
- .parse("INSERT INTO t2 (a,b) VALUES (1), (2,3)"))
- .isInstanceOf(ValidationException.class)
- .hasMessageContaining(" Number of columns must match number of query columns");
- }
-
- @Test
- void testInsertIntoShouldColumnMismatchWithMultiFieldValues() {
- assertThatThrownBy(
- () ->
- plannerMocks
- .getParser()
- .parse("INSERT INTO t2 (a,b) VALUES (1,2), (3,4,5)"))
+ @ParameterizedTest
+ @ValueSource(
+ strings = {
+ "INSERT INTO t2 (a, b) VALUES(1)",
+ "INSERT INTO t2 (a, b) VALUES (1, 2), (3)",
+ "INSERT INTO t2 (a, b) VALUES (1), (2, 3)",
+ "INSERT INTO t2 (a, b) VALUES (1, 2), (3, 4, 5)",
+ "INSERT INTO t2 (a, b) SELECT 1",
+ "INSERT INTO t2 (a, b) SELECT * FROM t1",
+ "INSERT INTO t2 (a, b) SELECT *, 42 FROM t2_copy",
+ "INSERT INTO t2 (a, b) SELECT 42, * FROM t2_copy",
+ "INSERT INTO t2 (a, b) SELECT * FROM t_nested",
+ "INSERT INTO t2 (a, b) TABLE t_nested",
+ "INSERT INTO t2 (a, b) SELECT * FROM (TABLE t_nested)"
+ })
+ void testInvalidNumberOfColumnsWhileInsertInto(String sql) {
+ assertThatThrownBy(() -> plannerMocks.getParser().parse(sql))
.isInstanceOf(ValidationException.class)
.hasMessageContaining(" Number of columns must match number of query columns");
}
- @Test
- void testInsertIntoShouldNotColumnMismatchWithValues() {
- assertDoesNotThrow(
- () -> {
- plannerMocks.getParser().parse("INSERT INTO t2 (a,b) VALUES (1,2), (3,4)");
- });
- }
-
- @Test
- void testInsertIntoShouldNotColumnMismatchWithSelect() {
- assertDoesNotThrow(
- () -> {
- plannerMocks.getParser().parse("INSERT INTO t2 (a,b) Select 1, 2");
- });
- }
-
- @Test
- void testInsertIntoShouldNotColumnMismatchWithSingleColValues() {
+ @ParameterizedTest
+ @ValueSource(
+ strings = {
+ "INSERT INTO t2 (a, b) VALUES (1, 2), (3, 4)",
+ "INSERT INTO t2 (a) VALUES (1), (3)",
+ "INSERT INTO t2 (a, b) SELECT 1, 2",
+ "INSERT INTO t2 (a, b) SELECT * FROM t2_copy",
+ "INSERT INTO t2 (a, b) SELECT *, 42 FROM t1",
+ "INSERT INTO t2 (a, b) SELECT 42, * FROM t1",
+ "INSERT INTO t2 (a, b) SELECT f.* FROM t_nested",
+ "INSERT INTO t2 (a, b) TABLE t2_copy"
Review Comment:
Interesting. In that case, how did you figure out that you needed to add that condition? (I'm mainly just curious.)
If it cannot be hit from SQL, it may be nice to see a unit test which exercises the new condition. Not sure how hard that'd be.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-36266][table] Insert into as select * behaves incorrect [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #25316:
URL: https://github.com/apache/flink/pull/25316#discussion_r1829801746
##########
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/calcite/FlinkCalciteSqlValidatorTest.java:
##########
@@ -50,74 +67,43 @@ void testUpsertInto() {
"UPSERT INTO statement is not supported. Please use INSERT INTO instead.");
}
- @Test
- void testInsertIntoShouldColumnMismatchWithValues() {
- assertThatThrownBy(() -> plannerMocks.getParser().parse("INSERT INTO t2 (a,b) VALUES(1)"))
- .isInstanceOf(ValidationException.class)
- .hasMessageContaining(" Number of columns must match number of query columns");
- }
-
- @Test
- void testInsertIntoShouldColumnMismatchWithSelect() {
- assertThatThrownBy(() -> plannerMocks.getParser().parse("INSERT INTO t2 (a,b) SELECT 1"))
- .isInstanceOf(ValidationException.class)
- .hasMessageContaining(" Number of columns must match number of query columns");
- }
-
- @Test
- void testInsertIntoShouldColumnMismatchWithLastValue() {
- assertThatThrownBy(
- () ->
- plannerMocks
- .getParser()
- .parse("INSERT INTO t2 (a,b) VALUES (1,2), (3)"))
- .isInstanceOf(ValidationException.class)
- .hasMessageContaining(" Number of columns must match number of query columns");
- }
-
- @Test
- void testInsertIntoShouldColumnMismatchWithFirstValue() {
- assertThatThrownBy(
- () ->
- plannerMocks
- .getParser()
- .parse("INSERT INTO t2 (a,b) VALUES (1), (2,3)"))
- .isInstanceOf(ValidationException.class)
- .hasMessageContaining(" Number of columns must match number of query columns");
- }
-
- @Test
- void testInsertIntoShouldColumnMismatchWithMultiFieldValues() {
- assertThatThrownBy(
- () ->
- plannerMocks
- .getParser()
- .parse("INSERT INTO t2 (a,b) VALUES (1,2), (3,4,5)"))
+ @ParameterizedTest
+ @ValueSource(
+ strings = {
+ "INSERT INTO t2 (a, b) VALUES(1)",
+ "INSERT INTO t2 (a, b) VALUES (1, 2), (3)",
+ "INSERT INTO t2 (a, b) VALUES (1), (2, 3)",
+ "INSERT INTO t2 (a, b) VALUES (1, 2), (3, 4, 5)",
+ "INSERT INTO t2 (a, b) SELECT 1",
+ "INSERT INTO t2 (a, b) SELECT * FROM t1",
+ "INSERT INTO t2 (a, b) SELECT *, 42 FROM t2_copy",
+ "INSERT INTO t2 (a, b) SELECT 42, * FROM t2_copy",
+ "INSERT INTO t2 (a, b) SELECT * FROM t_nested",
+ "INSERT INTO t2 (a, b) TABLE t_nested",
+ "INSERT INTO t2 (a, b) SELECT * FROM (TABLE t_nested)"
+ })
+ void testInvalidNumberOfColumnsWhileInsertInto(String sql) {
+ assertThatThrownBy(() -> plannerMocks.getParser().parse(sql))
.isInstanceOf(ValidationException.class)
.hasMessageContaining(" Number of columns must match number of query columns");
}
- @Test
- void testInsertIntoShouldNotColumnMismatchWithValues() {
- assertDoesNotThrow(
- () -> {
- plannerMocks.getParser().parse("INSERT INTO t2 (a,b) VALUES (1,2), (3,4)");
- });
- }
-
- @Test
- void testInsertIntoShouldNotColumnMismatchWithSelect() {
- assertDoesNotThrow(
- () -> {
- plannerMocks.getParser().parse("INSERT INTO t2 (a,b) Select 1, 2");
- });
- }
-
- @Test
- void testInsertIntoShouldNotColumnMismatchWithSingleColValues() {
+ @ParameterizedTest
+ @ValueSource(
+ strings = {
+ "INSERT INTO t2 (a, b) VALUES (1, 2), (3, 4)",
+ "INSERT INTO t2 (a) VALUES (1), (3)",
+ "INSERT INTO t2 (a, b) SELECT 1, 2",
+ "INSERT INTO t2 (a, b) SELECT * FROM t2_copy",
+ "INSERT INTO t2 (a, b) SELECT *, 42 FROM t1",
+ "INSERT INTO t2 (a, b) SELECT 42, * FROM t1",
+ "INSERT INTO t2 (a, b) SELECT f.* FROM t_nested",
+ "INSERT INTO t2 (a, b) TABLE t2_copy"
Review Comment:
after the fixing comment yes
now I tend to think to turn it to boolean
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-36266][table] Insert into as select * behaves incorrect [flink]
Posted by "jnh5y (via GitHub)" <gi...@apache.org>.
jnh5y commented on code in PR #25316:
URL: https://github.com/apache/flink/pull/25316#discussion_r1829644347
##########
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/calcite/FlinkCalciteSqlValidatorTest.java:
##########
@@ -50,74 +67,43 @@ void testUpsertInto() {
"UPSERT INTO statement is not supported. Please use INSERT INTO instead.");
}
- @Test
- void testInsertIntoShouldColumnMismatchWithValues() {
- assertThatThrownBy(() -> plannerMocks.getParser().parse("INSERT INTO t2 (a,b) VALUES(1)"))
- .isInstanceOf(ValidationException.class)
- .hasMessageContaining(" Number of columns must match number of query columns");
- }
-
- @Test
- void testInsertIntoShouldColumnMismatchWithSelect() {
- assertThatThrownBy(() -> plannerMocks.getParser().parse("INSERT INTO t2 (a,b) SELECT 1"))
- .isInstanceOf(ValidationException.class)
- .hasMessageContaining(" Number of columns must match number of query columns");
- }
-
- @Test
- void testInsertIntoShouldColumnMismatchWithLastValue() {
- assertThatThrownBy(
- () ->
- plannerMocks
- .getParser()
- .parse("INSERT INTO t2 (a,b) VALUES (1,2), (3)"))
- .isInstanceOf(ValidationException.class)
- .hasMessageContaining(" Number of columns must match number of query columns");
- }
-
- @Test
- void testInsertIntoShouldColumnMismatchWithFirstValue() {
- assertThatThrownBy(
- () ->
- plannerMocks
- .getParser()
- .parse("INSERT INTO t2 (a,b) VALUES (1), (2,3)"))
- .isInstanceOf(ValidationException.class)
- .hasMessageContaining(" Number of columns must match number of query columns");
- }
-
- @Test
- void testInsertIntoShouldColumnMismatchWithMultiFieldValues() {
- assertThatThrownBy(
- () ->
- plannerMocks
- .getParser()
- .parse("INSERT INTO t2 (a,b) VALUES (1,2), (3,4,5)"))
+ @ParameterizedTest
+ @ValueSource(
+ strings = {
+ "INSERT INTO t2 (a, b) VALUES(1)",
+ "INSERT INTO t2 (a, b) VALUES (1, 2), (3)",
+ "INSERT INTO t2 (a, b) VALUES (1), (2, 3)",
+ "INSERT INTO t2 (a, b) VALUES (1, 2), (3, 4, 5)",
+ "INSERT INTO t2 (a, b) SELECT 1",
+ "INSERT INTO t2 (a, b) SELECT * FROM t1",
+ "INSERT INTO t2 (a, b) SELECT *, 42 FROM t2_copy",
+ "INSERT INTO t2 (a, b) SELECT 42, * FROM t2_copy",
+ "INSERT INTO t2 (a, b) SELECT * FROM t_nested",
+ "INSERT INTO t2 (a, b) TABLE t_nested",
+ "INSERT INTO t2 (a, b) SELECT * FROM (TABLE t_nested)"
+ })
+ void testInvalidNumberOfColumnsWhileInsertInto(String sql) {
+ assertThatThrownBy(() -> plannerMocks.getParser().parse(sql))
.isInstanceOf(ValidationException.class)
.hasMessageContaining(" Number of columns must match number of query columns");
}
- @Test
- void testInsertIntoShouldNotColumnMismatchWithValues() {
- assertDoesNotThrow(
- () -> {
- plannerMocks.getParser().parse("INSERT INTO t2 (a,b) VALUES (1,2), (3,4)");
- });
- }
-
- @Test
- void testInsertIntoShouldNotColumnMismatchWithSelect() {
- assertDoesNotThrow(
- () -> {
- plannerMocks.getParser().parse("INSERT INTO t2 (a,b) Select 1, 2");
- });
- }
-
- @Test
- void testInsertIntoShouldNotColumnMismatchWithSingleColValues() {
+ @ParameterizedTest
+ @ValueSource(
+ strings = {
+ "INSERT INTO t2 (a, b) VALUES (1, 2), (3, 4)",
+ "INSERT INTO t2 (a) VALUES (1), (3)",
+ "INSERT INTO t2 (a, b) SELECT 1, 2",
+ "INSERT INTO t2 (a, b) SELECT * FROM t2_copy",
+ "INSERT INTO t2 (a, b) SELECT *, 42 FROM t1",
+ "INSERT INTO t2 (a, b) SELECT 42, * FROM t1",
+ "INSERT INTO t2 (a, b) SELECT f.* FROM t_nested",
+ "INSERT INTO t2 (a, b) TABLE t2_copy"
Review Comment:
Is this the case which catches the condition `identifiersSize == 0` above? (Asking for my understanding.)
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-36266][table] Insert into as select * behaves incorrect [flink]
Posted by "jnh5y (via GitHub)" <gi...@apache.org>.
jnh5y commented on code in PR #25316:
URL: https://github.com/apache/flink/pull/25316#discussion_r1830289614
##########
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/calcite/FlinkCalciteSqlValidatorTest.java:
##########
@@ -109,7 +109,20 @@ void testInvalidNumberOfColumnsWhileInsertInto(String sql) {
"INSERT INTO t2 (a, b) WITH cte AS (SELECT 1, 2) SELECT * FROM cte",
"INSERT INTO t2 (a, b) WITH cte AS (SELECT * FROM t2_copy) SELECT * FROM cte",
"INSERT INTO t2 (a, b) WITH cte AS (SELECT t1.a, t2_copy.b FROM t1, t2_copy) SELECT * FROM cte",
- "INSERT INTO t2 (a, b) WITH cte1 AS (SELECT 1), cte2 AS (SELECT 2) SELECT * FROM cte1, cte2"
+ "INSERT INTO t2 (a, b) WITH cte1 AS (SELECT 1), cte2 AS (SELECT 2) SELECT * FROM cte1, cte2",
+ "INSERT INTO t2 (a, b) "
+ + "WITH cte1 AS (SELECT 1, 2), "
+ + "cte2 AS (SELECT 2, 3) "
+ + "SELECT * FROM cte1 UNION SELECT * FROM cte2",
Review Comment:
There are failing unit tests with CTEs above. Do we want some UNIONs which do not work above? Definitely a non-blocking suggestion.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-36266][table] Insert into as select * behaves incorrect [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on PR #25316:
URL: https://github.com/apache/flink/pull/25316#issuecomment-2439730805
@flinkbot run azure
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-36266][table] Insert into as select * behaves incorrect [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #25316:
URL: https://github.com/apache/flink/pull/25316#discussion_r1830063028
##########
flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/calcite/SqlRewriterUtils.scala:
##########
@@ -173,69 +207,73 @@ object SqlRewriterUtils {
// Expands the select list first in case there is a star(*).
// Validates the select first to register the where scope.
validator.validate(select)
- val sourceList = validator.expandStar(select.getSelectList, select, false).getList
+ reorderAndValidateForSelect(validator, select, targetRowType, assignedFields, targetPosition)
+ select
+ }
+ def rewriteSqlValues(
+ values: SqlCall,
+ targetRowType: RelDataType,
+ assignedFields: util.LinkedHashMap[Integer, SqlNode],
+ targetPosition: util.List[Int]): SqlCall = {
val fixedNodes = new util.ArrayList[SqlNode]
+ (0 until values.getOperandList.size()).foreach {
+ valueIdx =>
+ val value = values.getOperandList.get(valueIdx)
+ val valueAsList = if (value.getKind == SqlKind.ROW) {
+ value.asInstanceOf[SqlCall].getOperandList
+ } else {
+ Collections.singletonList(value)
+ }
+ val nodes = getReorderedNodes(targetRowType, assignedFields, targetPosition, valueAsList)
+ fixedNodes.add(SqlStdOperatorTable.ROW.createCall(value.getParserPosition, nodes))
+ }
+ SqlStdOperatorTable.VALUES.createCall(values.getParserPosition, fixedNodes)
+ }
+
+ private def getReorderedNodes(
Review Comment:
Here mostly extracting common logic in one place to reduce code duplication
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-36266][table] Insert into as select * behaves incorrect [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #25316:
URL: https://github.com/apache/flink/pull/25316#discussion_r1829780200
##########
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/calcite/FlinkCalciteSqlValidatorTest.java:
##########
@@ -50,74 +67,43 @@ void testUpsertInto() {
"UPSERT INTO statement is not supported. Please use INSERT INTO instead.");
}
- @Test
- void testInsertIntoShouldColumnMismatchWithValues() {
- assertThatThrownBy(() -> plannerMocks.getParser().parse("INSERT INTO t2 (a,b) VALUES(1)"))
- .isInstanceOf(ValidationException.class)
- .hasMessageContaining(" Number of columns must match number of query columns");
- }
-
- @Test
- void testInsertIntoShouldColumnMismatchWithSelect() {
- assertThatThrownBy(() -> plannerMocks.getParser().parse("INSERT INTO t2 (a,b) SELECT 1"))
- .isInstanceOf(ValidationException.class)
- .hasMessageContaining(" Number of columns must match number of query columns");
- }
-
- @Test
- void testInsertIntoShouldColumnMismatchWithLastValue() {
- assertThatThrownBy(
- () ->
- plannerMocks
- .getParser()
- .parse("INSERT INTO t2 (a,b) VALUES (1,2), (3)"))
- .isInstanceOf(ValidationException.class)
- .hasMessageContaining(" Number of columns must match number of query columns");
- }
-
- @Test
- void testInsertIntoShouldColumnMismatchWithFirstValue() {
- assertThatThrownBy(
- () ->
- plannerMocks
- .getParser()
- .parse("INSERT INTO t2 (a,b) VALUES (1), (2,3)"))
- .isInstanceOf(ValidationException.class)
- .hasMessageContaining(" Number of columns must match number of query columns");
- }
-
- @Test
- void testInsertIntoShouldColumnMismatchWithMultiFieldValues() {
- assertThatThrownBy(
- () ->
- plannerMocks
- .getParser()
- .parse("INSERT INTO t2 (a,b) VALUES (1,2), (3,4,5)"))
+ @ParameterizedTest
+ @ValueSource(
+ strings = {
+ "INSERT INTO t2 (a, b) VALUES(1)",
+ "INSERT INTO t2 (a, b) VALUES (1, 2), (3)",
+ "INSERT INTO t2 (a, b) VALUES (1), (2, 3)",
+ "INSERT INTO t2 (a, b) VALUES (1, 2), (3, 4, 5)",
+ "INSERT INTO t2 (a, b) SELECT 1",
+ "INSERT INTO t2 (a, b) SELECT * FROM t1",
+ "INSERT INTO t2 (a, b) SELECT *, 42 FROM t2_copy",
+ "INSERT INTO t2 (a, b) SELECT 42, * FROM t2_copy",
+ "INSERT INTO t2 (a, b) SELECT * FROM t_nested",
+ "INSERT INTO t2 (a, b) TABLE t_nested",
+ "INSERT INTO t2 (a, b) SELECT * FROM (TABLE t_nested)"
+ })
+ void testInvalidNumberOfColumnsWhileInsertInto(String sql) {
+ assertThatThrownBy(() -> plannerMocks.getParser().parse(sql))
.isInstanceOf(ValidationException.class)
.hasMessageContaining(" Number of columns must match number of query columns");
}
- @Test
- void testInsertIntoShouldNotColumnMismatchWithValues() {
- assertDoesNotThrow(
- () -> {
- plannerMocks.getParser().parse("INSERT INTO t2 (a,b) VALUES (1,2), (3,4)");
- });
- }
-
- @Test
- void testInsertIntoShouldNotColumnMismatchWithSelect() {
- assertDoesNotThrow(
- () -> {
- plannerMocks.getParser().parse("INSERT INTO t2 (a,b) Select 1, 2");
- });
- }
-
- @Test
- void testInsertIntoShouldNotColumnMismatchWithSingleColValues() {
+ @ParameterizedTest
+ @ValueSource(
+ strings = {
+ "INSERT INTO t2 (a, b) VALUES (1, 2), (3, 4)",
+ "INSERT INTO t2 (a) VALUES (1), (3)",
+ "INSERT INTO t2 (a, b) SELECT 1, 2",
+ "INSERT INTO t2 (a, b) SELECT * FROM t2_copy",
+ "INSERT INTO t2 (a, b) SELECT *, 42 FROM t1",
+ "INSERT INTO t2 (a, b) SELECT 42, * FROM t1",
+ "INSERT INTO t2 (a, b) SELECT f.* FROM t_nested",
+ "INSERT INTO t2 (a, b) TABLE t2_copy"
Review Comment:
Seems I got what you mean
from one side there was a wrong condition in filter, fix that, thanks for catching
from another side currently there are 2 checks
one at [SqlIdentifier](https://github.com/apache/flink/pull/25316/files#diff-12ede804afb130bfe9c83df9da2ac5745b14345d7a71bee0e803257e6df73951R136)
another at https://github.com/apache/flink/pull/25316/files#diff-12ede804afb130bfe9c83df9da2ac5745b14345d7a71bee0e803257e6df73951R187
the difference is that first could be executed immediately (kind of short circuit)
second will be executed only after validation and star expansion...
We can remove the first one however it means that for all queries we will do first validation check and then column number check even if there is no star
WDYT?
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-36266][table] Insert into as select * behaves incorrect [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on PR #25316:
URL: https://github.com/apache/flink/pull/25316#issuecomment-2407516038
@flinkbot run azure
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-36266][table] Insert into as select * behaves incorrect [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #25316:
URL: https://github.com/apache/flink/pull/25316#discussion_r1829676623
##########
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/calcite/FlinkCalciteSqlValidatorTest.java:
##########
@@ -50,74 +67,43 @@ void testUpsertInto() {
"UPSERT INTO statement is not supported. Please use INSERT INTO instead.");
}
- @Test
- void testInsertIntoShouldColumnMismatchWithValues() {
- assertThatThrownBy(() -> plannerMocks.getParser().parse("INSERT INTO t2 (a,b) VALUES(1)"))
- .isInstanceOf(ValidationException.class)
- .hasMessageContaining(" Number of columns must match number of query columns");
- }
-
- @Test
- void testInsertIntoShouldColumnMismatchWithSelect() {
- assertThatThrownBy(() -> plannerMocks.getParser().parse("INSERT INTO t2 (a,b) SELECT 1"))
- .isInstanceOf(ValidationException.class)
- .hasMessageContaining(" Number of columns must match number of query columns");
- }
-
- @Test
- void testInsertIntoShouldColumnMismatchWithLastValue() {
- assertThatThrownBy(
- () ->
- plannerMocks
- .getParser()
- .parse("INSERT INTO t2 (a,b) VALUES (1,2), (3)"))
- .isInstanceOf(ValidationException.class)
- .hasMessageContaining(" Number of columns must match number of query columns");
- }
-
- @Test
- void testInsertIntoShouldColumnMismatchWithFirstValue() {
- assertThatThrownBy(
- () ->
- plannerMocks
- .getParser()
- .parse("INSERT INTO t2 (a,b) VALUES (1), (2,3)"))
- .isInstanceOf(ValidationException.class)
- .hasMessageContaining(" Number of columns must match number of query columns");
- }
-
- @Test
- void testInsertIntoShouldColumnMismatchWithMultiFieldValues() {
- assertThatThrownBy(
- () ->
- plannerMocks
- .getParser()
- .parse("INSERT INTO t2 (a,b) VALUES (1,2), (3,4,5)"))
+ @ParameterizedTest
+ @ValueSource(
+ strings = {
+ "INSERT INTO t2 (a, b) VALUES(1)",
+ "INSERT INTO t2 (a, b) VALUES (1, 2), (3)",
+ "INSERT INTO t2 (a, b) VALUES (1), (2, 3)",
+ "INSERT INTO t2 (a, b) VALUES (1, 2), (3, 4, 5)",
+ "INSERT INTO t2 (a, b) SELECT 1",
+ "INSERT INTO t2 (a, b) SELECT * FROM t1",
+ "INSERT INTO t2 (a, b) SELECT *, 42 FROM t2_copy",
+ "INSERT INTO t2 (a, b) SELECT 42, * FROM t2_copy",
+ "INSERT INTO t2 (a, b) SELECT * FROM t_nested",
+ "INSERT INTO t2 (a, b) TABLE t_nested",
+ "INSERT INTO t2 (a, b) SELECT * FROM (TABLE t_nested)"
+ })
+ void testInvalidNumberOfColumnsWhileInsertInto(String sql) {
+ assertThatThrownBy(() -> plannerMocks.getParser().parse(sql))
.isInstanceOf(ValidationException.class)
.hasMessageContaining(" Number of columns must match number of query columns");
}
- @Test
- void testInsertIntoShouldNotColumnMismatchWithValues() {
- assertDoesNotThrow(
- () -> {
- plannerMocks.getParser().parse("INSERT INTO t2 (a,b) VALUES (1,2), (3,4)");
- });
- }
-
- @Test
- void testInsertIntoShouldNotColumnMismatchWithSelect() {
- assertDoesNotThrow(
- () -> {
- plannerMocks.getParser().parse("INSERT INTO t2 (a,b) Select 1, 2");
- });
- }
-
- @Test
- void testInsertIntoShouldNotColumnMismatchWithSingleColValues() {
+ @ParameterizedTest
+ @ValueSource(
+ strings = {
+ "INSERT INTO t2 (a, b) VALUES (1, 2), (3, 4)",
+ "INSERT INTO t2 (a) VALUES (1), (3)",
+ "INSERT INTO t2 (a, b) SELECT 1, 2",
+ "INSERT INTO t2 (a, b) SELECT * FROM t2_copy",
+ "INSERT INTO t2 (a, b) SELECT *, 42 FROM t1",
+ "INSERT INTO t2 (a, b) SELECT 42, * FROM t1",
+ "INSERT INTO t2 (a, b) SELECT f.* FROM t_nested",
+ "INSERT INTO t2 (a, b) TABLE t2_copy"
Review Comment:
i guess no
here are only tests with SQL
do you have an idea how SQL would look like for the case
>identifiersSize == 0
?
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-36266][table] Insert into as select * behaves incorrect [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #25316:
URL: https://github.com/apache/flink/pull/25316#discussion_r1829790186
##########
flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/calcite/SqlRewriterUtils.scala:
##########
@@ -157,9 +161,14 @@ object SqlRewriterUtils {
operands.get(1).asInstanceOf[SqlNodeList],
operands.get(2),
operands.get(3))
+ case SqlKind.EXPLICIT_TABLE =>
+ val operands = call.getOperandList
+ val expTable = new ExplicitTableSqlSelect(
+ operands.get(0).asInstanceOf[SqlIdentifier],
+ Collections.emptyList())
+ rewriterUtils.rewriteSelect(expTable, targetRowType, assignedFields, targetPosition)
// Not support:
// case SqlKind.WITH =>
Review Comment:
looks like for that first should be supported in Calcite
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-36266][table] Insert into as select * behaves incorrect [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #25316:
URL: https://github.com/apache/flink/pull/25316#discussion_r1829788023
##########
flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/calcite/SqlRewriterUtils.scala:
##########
@@ -131,7 +131,7 @@ object SqlRewriterUtils {
call.getKind match {
case SqlKind.SELECT =>
val sqlSelect = call.asInstanceOf[SqlSelect]
- val identifiersSize = sqlSelect.getSelectList.count(s => s.isInstanceOf[SqlIdentifier])
+ val identifiersSize = sqlSelect.getSelectList.count(s => !SqlIdentifier.STAR.equals(s))
Review Comment:
sure, 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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-36266][table] Insert into as select * behaves incorrect [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin merged PR #25316:
URL: https://github.com/apache/flink/pull/25316
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-36266][table] Insert into as select * behaves incorrect [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on PR #25316:
URL: https://github.com/apache/flink/pull/25316#issuecomment-2438607426
@flinkbot run azure
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-36266][table] Insert into as select * behaves incorrect [flink]
Posted by "jnh5y (via GitHub)" <gi...@apache.org>.
jnh5y commented on code in PR #25316:
URL: https://github.com/apache/flink/pull/25316#discussion_r1830289157
##########
flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/calcite/SqlRewriterUtils.scala:
##########
@@ -193,11 +193,33 @@ object SqlRewriterUtils {
// Expands the select list first in case there is a star(*).
// Validates the select first to register the where scope.
validator.validate(cte)
- val select = cte.body.asInstanceOf[SqlSelect];
- reorderAndValidateForSelect(validator, select, targetRowType, assignedFields, targetPosition)
+ val selects = new util.ArrayList[SqlSelect]()
+ extractSelectsFromCte(cte.body.asInstanceOf[SqlCall], selects)
+
+ for (select <- selects) {
+ reorderAndValidateForSelect(validator, select, targetRowType, assignedFields, targetPosition)
+ }
cte
}
+ def extractSelectsFromCte(cte: SqlCall, selects: util.List[SqlSelect]): Unit = {
Review Comment:
Nice!
I kinda don't like that `selects` is passed in instead of returned. I tried refactoring it, but it doesn't look much better.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-36266][table] Insert into as select * behaves incorrect [flink]
Posted by "twalthr (via GitHub)" <gi...@apache.org>.
twalthr commented on code in PR #25316:
URL: https://github.com/apache/flink/pull/25316#discussion_r1829717408
##########
flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/calcite/SqlRewriterUtils.scala:
##########
@@ -130,8 +131,11 @@ object SqlRewriterUtils {
call.getKind match {
case SqlKind.SELECT =>
val sqlSelect = call.asInstanceOf[SqlSelect]
-
- if (targetPosition.nonEmpty && sqlSelect.getSelectList.size() != targetPosition.size()) {
+ val identifiersSize = sqlSelect.getSelectList.count(s => s.isInstanceOf[SqlIdentifier])
+ if (
+ identifiersSize == 0
Review Comment:
I have a similar question than Jim. What is this line good for? maybe an inline comment makes sense here with an example?
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-36266][table] Insert into as select * behaves incorrect [flink]
Posted by "jnh5y (via GitHub)" <gi...@apache.org>.
jnh5y commented on code in PR #25316:
URL: https://github.com/apache/flink/pull/25316#discussion_r1829778233
##########
flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/calcite/SqlRewriterUtils.scala:
##########
@@ -131,7 +131,7 @@ object SqlRewriterUtils {
call.getKind match {
case SqlKind.SELECT =>
val sqlSelect = call.asInstanceOf[SqlSelect]
- val identifiersSize = sqlSelect.getSelectList.count(s => s.isInstanceOf[SqlIdentifier])
+ val identifiersSize = sqlSelect.getSelectList.count(s => !SqlIdentifier.STAR.equals(s))
Review Comment:
Interesting. This change makes me think that the columns could some computation. Is it worth testing that case?
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-36266][table] Insert into as select * behaves incorrect [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #25316:
URL: https://github.com/apache/flink/pull/25316#discussion_r1830498758
##########
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/calcite/FlinkCalciteSqlValidatorTest.java:
##########
@@ -109,7 +109,20 @@ void testInvalidNumberOfColumnsWhileInsertInto(String sql) {
"INSERT INTO t2 (a, b) WITH cte AS (SELECT 1, 2) SELECT * FROM cte",
"INSERT INTO t2 (a, b) WITH cte AS (SELECT * FROM t2_copy) SELECT * FROM cte",
"INSERT INTO t2 (a, b) WITH cte AS (SELECT t1.a, t2_copy.b FROM t1, t2_copy) SELECT * FROM cte",
- "INSERT INTO t2 (a, b) WITH cte1 AS (SELECT 1), cte2 AS (SELECT 2) SELECT * FROM cte1, cte2"
+ "INSERT INTO t2 (a, b) WITH cte1 AS (SELECT 1), cte2 AS (SELECT 2) SELECT * FROM cte1, cte2",
+ "INSERT INTO t2 (a, b) "
+ + "WITH cte1 AS (SELECT 1, 2), "
+ + "cte2 AS (SELECT 2, 3) "
+ + "SELECT * FROM cte1 UNION SELECT * FROM cte2",
Review Comment:
With latest commit all tests passed
so shouldn't be an issue I guess
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-36266][table] Insert into as select * behaves incorrect [flink]
Posted by "jnh5y (via GitHub)" <gi...@apache.org>.
jnh5y commented on code in PR #25316:
URL: https://github.com/apache/flink/pull/25316#discussion_r1831311820
##########
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/calcite/FlinkCalciteSqlValidatorTest.java:
##########
@@ -109,7 +109,20 @@ void testInvalidNumberOfColumnsWhileInsertInto(String sql) {
"INSERT INTO t2 (a, b) WITH cte AS (SELECT 1, 2) SELECT * FROM cte",
"INSERT INTO t2 (a, b) WITH cte AS (SELECT * FROM t2_copy) SELECT * FROM cte",
"INSERT INTO t2 (a, b) WITH cte AS (SELECT t1.a, t2_copy.b FROM t1, t2_copy) SELECT * FROM cte",
- "INSERT INTO t2 (a, b) WITH cte1 AS (SELECT 1), cte2 AS (SELECT 2) SELECT * FROM cte1, cte2"
+ "INSERT INTO t2 (a, b) WITH cte1 AS (SELECT 1), cte2 AS (SELECT 2) SELECT * FROM cte1, cte2",
+ "INSERT INTO t2 (a, b) "
+ + "WITH cte1 AS (SELECT 1, 2), "
+ + "cte2 AS (SELECT 2, 3) "
+ + "SELECT * FROM cte1 UNION SELECT * FROM cte2",
Review Comment:
Sounds good!
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-36266][table] Insert into as select * behaves incorrect [flink]
Posted by "flinkbot (via GitHub)" <gi...@apache.org>.
flinkbot commented on PR #25316:
URL: https://github.com/apache/flink/pull/25316#issuecomment-2344986515
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "bd1b0ce7b547c267e02c754607f663142dad4477",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "bd1b0ce7b547c267e02c754607f663142dad4477",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* bd1b0ce7b547c267e02c754607f663142dad4477 UNKNOWN
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run azure` re-run the last Azure build
</details>
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org