You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "ludlows (via GitHub)" <gi...@apache.org> on 2024/10/31 07:48:23 UTC
[PR] Spark-3.5: make `where` sql case sensitive setting alterable in rewrite data files procedure [iceberg]
ludlows opened a new pull request, #11439:
URL: https://github.com/apache/iceberg/pull/11439
this pr aims to make the rewriteDataFile action is aware of the user settings about sql case sensitivity in the `where` statement.
the implementation is simple.
we first obtain the case sensitive setting and save it as a variable in the custructor of rewriteDataFileAction.
then, we pass the variable to the tableScan .
related issue: https://github.com/apache/iceberg/issues/11438
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark-3.5: make `where` sql case sensitive setting alterable in rewrite data files procedure [iceberg]
Posted by "ludlows (via GitHub)" <gi...@apache.org>.
ludlows commented on PR #11439:
URL: https://github.com/apache/iceberg/pull/11439#issuecomment-2509767926
@huaxingao thanks for the comment.
but i don't think the problem will be raised since the bug has been fixed by this PR.
please check the test code belove:
```java
@TestTemplate
public void testFilterCaseSensitivityAfterChange() {
createTable();
insertData(10);
sql("set spark.sql.caseSensitive=false");
assertEquals(
"Should have done nothing but passed the schema validation, since no files are present",
ImmutableList.of(row(0, 0, 0L, 0)),
sql(
"CALL %s.system.rewrite_data_files(table=>'%s', where=>'C1 > 90000000')",
catalogName, tableIdent));
}
```
the test case above has passed .
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark-3.5: make `where` sql case sensitive setting alterable in rewrite data files procedure [iceberg]
Posted by "ludlows (via GitHub)" <gi...@apache.org>.
ludlows commented on PR #11439:
URL: https://github.com/apache/iceberg/pull/11439#issuecomment-2456884157
I think I typed the wrong version of iceberg in the issue https://github.com/apache/iceberg/issues/11438
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark-3.5: make `where` sql case sensitive setting alterable in rewrite data files procedure [iceberg]
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao commented on PR #11439:
URL: https://github.com/apache/iceberg/pull/11439#issuecomment-2507166143
@ludlows Thanks for the quick fix. Can we have a test that fails without the fix but passes with it? It seems that all your current tests pass even without the fix.
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark-3.5: make `where` sql case sensitive setting alterable in rewrite data files procedure [iceberg]
Posted by "ludlows (via GitHub)" <gi...@apache.org>.
ludlows commented on PR #11439:
URL: https://github.com/apache/iceberg/pull/11439#issuecomment-2456878191
Hi @szehon-ho ,
please review the test cases, should you have time.
one possible problem is the type of exception is `IllegalArgumentException` here instead of the `ValidationException` mentioned in the issue.
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark-3.5: make `where` sql case sensitive setting alterable in rewrite data files procedure [iceberg]
Posted by "szehon-ho (via GitHub)" <gi...@apache.org>.
szehon-ho merged PR #11439:
URL: https://github.com/apache/iceberg/pull/11439
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark-3.5: make `where` sql case sensitive setting alterable in rewrite data files procedure [iceberg]
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao commented on code in PR #11439:
URL: https://github.com/apache/iceberg/pull/11439#discussion_r1830369497
##########
spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestRewriteDataFilesProcedure.java:
##########
@@ -69,6 +69,60 @@ public void removeTable() {
sql("DROP TABLE IF EXISTS %s", tableName(QUOTED_SPECIAL_CHARS_TABLE_NAME));
}
+ @TestTemplate
+ public void testFailsByCaseSensitiveWhereSql() {
+ createTable();
+ insertData(10);
+ sql("set spark.sql.caseSensitive=true");
+ assertThatThrownBy(
+ () ->
+ sql(
+ "CALL %s.system.rewrite_data_files(table=>'%s', where=>'C1 > 0')",
+ catalogName, tableIdent))
Review Comment:
When `spark.sql.caseSensitive` is set to true, this will fail at the Spark analyzer regardless of the changes in this PR.
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark-3.5: make `where` sql case sensitive setting alterable in rewrite data files procedure [iceberg]
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao commented on PR #11439:
URL: https://github.com/apache/iceberg/pull/11439#issuecomment-2458700123
@ludlows
Thanks for the PR. I have a couple of questions:
1. It seems to me that the tests don't really test the changes in this PR; they would pass even without the fix. I think we should add some tests that would fail without the fix but can pass with it.
3. Do we want to make the Spark SQL configuration `spark.sql.caseSensitive` apply to Iceberg stored procedure parameters? If so, we probably should apply `spark.sql.caseSensitive` to all Iceberg stored procedure parameters. Are there other Iceberg stored procedure parameters that should also honor `spark.sql.caseSensitive`?
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark-3.5: make `where` sql case sensitive setting alterable in rewrite data files procedure [iceberg]
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao commented on PR #11439:
URL: https://github.com/apache/iceberg/pull/11439#issuecomment-2509628202
@ludlows I think you can simply reproduce the problem by something like
```
createTable();
insertData(10);
sql("SET %s=false", SQLConf.CASE_SENSITIVE().key());
sql("CALL %s.system.rewrite_data_files(table=>'%s', where=>'C1 > 0'), catalogName, tableIdent));
```
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark-3.5: make `where` sql case sensitive setting alterable in rewrite data files procedure [iceberg]
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao commented on code in PR #11439:
URL: https://github.com/apache/iceberg/pull/11439#discussion_r1861308936
##########
spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteDataFilesSparkAction.java:
##########
@@ -102,11 +102,13 @@ public class RewriteDataFilesSparkAction
private boolean useStartingSequenceNumber;
private RewriteJobOrder rewriteJobOrder;
private FileRewriter<FileScanTask, DataFile> rewriter = null;
+ private boolean caseSensitive;
RewriteDataFilesSparkAction(SparkSession spark, Table table) {
super(spark.cloneSession());
// Disable Adaptive Query Execution as this may change the output partitioning of our write
spark().conf().set(SQLConf.ADAPTIVE_EXECUTION_ENABLED().key(), false);
+ this.caseSensitive = (boolean) spark().conf().get(SQLConf.CASE_SENSITIVE(), true);
Review Comment:
Can we use `SparkUtil.caseSensitive(spark)` instead?
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark-3.5: make `where` sql case sensitive setting alterable in rewrite data files procedure [iceberg]
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao commented on code in PR #11439:
URL: https://github.com/apache/iceberg/pull/11439#discussion_r1830370555
##########
spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestRewriteDataFilesProcedure.java:
##########
@@ -69,6 +69,60 @@ public void removeTable() {
sql("DROP TABLE IF EXISTS %s", tableName(QUOTED_SPECIAL_CHARS_TABLE_NAME));
}
+ @TestTemplate
+ public void testFailsByCaseSensitiveWhereSql() {
+ createTable();
+ insertData(10);
+ sql("set spark.sql.caseSensitive=true");
+ assertThatThrownBy(
+ () ->
+ sql(
+ "CALL %s.system.rewrite_data_files(table=>'%s', where=>'C1 > 0')",
+ catalogName, tableIdent))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Cannot parse predicates in where option: C1 > 0");
+ }
+
+ @TestTemplate
+ public void testSucceedByCaseInsensitiveWhereSql() {
+ createTable();
+ sql("set spark.sql.caseSensitive=false");
+ assertEquals(
+ "Should have done nothing but passed the schema validation, since no files are present",
+ ImmutableList.of(row(0, 0, 0L, 0)),
+ sql(
+ "CALL %s.system.rewrite_data_files(table=>'%s', where=>'C1 > 0')",
Review Comment:
With `spark.sql.caseSensitive` set to false, the schema validation will always pass, with or without the changes in this PR.
I think we want to add tests that will fail without the fix and pass with the fix.
We can add a test with `insertData` and `spark.sql.caseSensitive=false`. I think that test will fail without the fix and pass with the fix.
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark-3.5: make `where` sql case sensitive setting alterable in rewrite data files procedure [iceberg]
Posted by "ludlows (via GitHub)" <gi...@apache.org>.
ludlows commented on PR #11439:
URL: https://github.com/apache/iceberg/pull/11439#issuecomment-2459142794
1. It seems to me that the tests don't really test the changes in this PR; they would pass even without the fix. I think we should add some tests that would fail without the fix but can pass with it.
2. Do we want to make the Spark SQL configuration spark.sql.caseSensitive apply to Iceberg stored procedure parameters? If so, we probably should apply spark.sql.caseSensitive to all Iceberg stored procedure parameters. Are there other Iceberg stored procedure parameters that should also honor spark.sql.caseSensitive?
----------------------------------
hi @huaxingao ,
thanks for the questions above.
about the 2nd one, as a data engineer, our users were asking me : why not all parts in the procedure are case insensitive even I have set spark.sql.caseSensitive to false since the procedure is trigger at the sql level but the parameter of procedure is not affected?
I think it is reasonable to apply the setting about the sql case sensitivity to all procedures, but we could take this pr as the starting point.
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark-3.5: make `where` sql case sensitive setting alterable in rewrite data files procedure [iceberg]
Posted by "ludlows (via GitHub)" <gi...@apache.org>.
ludlows commented on PR #11439:
URL: https://github.com/apache/iceberg/pull/11439#issuecomment-2508880983
@huaxingao I think the test method `testFilterCaseSensitivityBeforeChange()` (leading to validation exception) has shown the bug exists before the PR.
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark-3.5: make `where` sql case sensitive setting alterable in rewrite data files procedure [iceberg]
Posted by "szehon-ho (via GitHub)" <gi...@apache.org>.
szehon-ho commented on PR #11439:
URL: https://github.com/apache/iceberg/pull/11439#issuecomment-2513022061
Thanks @ludlows , and also @huaxingao, @anuragmantri @singhpk234 for reviews
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark-3.5: make `where` sql case sensitive setting alterable in rewrite data files procedure [iceberg]
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao commented on PR #11439:
URL: https://github.com/apache/iceberg/pull/11439#issuecomment-2510198274
@ludlows Thanks for the quick reply. I know my example will pass with the PR's fix. However, the problem will arise without the fix. We need a simple test that fails without the fix and passes with it. A straightforward test like my example should suffice, with minimal changes. My goal is to keep the test as simple as possible.
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark-3.5: make `where` sql case sensitive setting alterable in rewrite data files procedure [iceberg]
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao commented on code in PR #11439:
URL: https://github.com/apache/iceberg/pull/11439#discussion_r1865206027
##########
spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteDataFilesSparkAction.java:
##########
@@ -153,6 +156,11 @@ public RewriteDataFilesSparkAction filter(Expression expression) {
return this;
}
+ public RewriteDataFilesSparkAction setCaseSensitive(boolean caseSensitiveReset) {
Review Comment:
This method is not used. Remove this unnecessary change?
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org