szehon-ho commented on code in PR #7732:
URL: https://github.com/apache/iceberg/pull/7732#discussion_r1847158756
##########
spark/v3.4/spark/src/main/java/org/apache/iceberg/spark/source/IcebergSource.java:
##########
@@ -61,7 +62,8 @@
* <p>The above list is in order of priority. For example: a matching catalog
will take priority
* over any namespace resolution.
*/
-public class IcebergSource implements DataSourceRegister,
SupportsCatalogOptions {
+public class IcebergSource
Review Comment:
Another comment, is it now multiple ways to configure properties (including
https://github.com/apache/iceberg/pull/4011), it may be confusing to user.
Worth to add a documentation about it, listing the precedence, ie:
I guess using dataframe API (to be double-checked)
- explicit dataframe option
- dataframe session default
- if table exists, explicit table option
- if table exists, table default
##########
spark/v3.4/spark/src/test/java/org/apache/iceberg/spark/source/TestIcebergSourceTablesBase.java:
##########
@@ -1953,6 +1953,65 @@ public void testTableWithInt96Timestamp() throws
IOException {
}
}
+ @Test
+ public void testSessionConfigSupport() {
+ PartitionSpec spec =
PartitionSpec.builderFor(SCHEMA).identity("id").build();
+ TableIdentifier tableIdentifier = TableIdentifier.of("db",
"session_config_table");
+ Table table = createTable(tableIdentifier, SCHEMA, spec);
+
+ List<SimpleRecord> records =
+ Lists.newArrayList(
+ new SimpleRecord(1, "a"), new SimpleRecord(2, "b"), new
SimpleRecord(3, "c"));
+
+ Dataset<Row> df = spark.createDataFrame(records, SimpleRecord.class);
+
+ df.select("id", "data")
+ .write()
+ .format("iceberg")
+ .mode(SaveMode.Append)
+ .save(loadLocation(tableIdentifier));
+
+ long snapshotId = table.currentSnapshot().snapshotId();
+
+ withSQLConf(
+ // set write option through session configuration
+ ImmutableMap.of("spark.datasource.iceberg.overwrite-mode", "dynamic"),
Review Comment:
Yea sorry, i was not clear. Its just a suggestion.
I think, because the test is to test SessionConfigSupport functionality
only, it may be more clear for the reader if its like:
```
'spark.datasource.iceberg.snapshot-property.foo=bar'
and then check if foo is set on latest snapshot summary?
```
Because i think the reader of the test need to know what is 'dynamic
overwrite' mode to understand the assert (its not related to the feature),
whereas the above is a bit more self-explanatory imo.
--
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-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
