Abacn commented on code in PR #37539:
URL: https://github.com/apache/beam/pull/37539#discussion_r3076635925
##########
sdks/java/extensions/sql/iceberg/src/main/java/org/apache/beam/sdk/extensions/sql/meta/provider/iceberg/IcebergTable.java:
##########
@@ -71,9 +74,21 @@ class IcebergTable extends SchemaBaseBeamTable {
this.tableIdentifier = tableIdentifier;
this.catalogConfig = catalogConfig;
ObjectNode properties = table.getProperties();
- if (properties.has(TRIGGERING_FREQUENCY_FIELD)) {
- this.triggeringFrequency =
properties.get(TRIGGERING_FREQUENCY_FIELD).asInt();
+ for (Map.Entry<String, JsonNode> property : properties.properties()) {
+ String name = property.getKey();
+ if (name.startsWith(BEAM_WRITE_PROPERTY)) {
+ String prop = name.substring(BEAM_WRITE_PROPERTY.length());
+ if (prop.equals(TRIGGERING_FREQUENCY_FIELD)) {
+ this.triggeringFrequency = property.getValue().asInt();
+ } else {
+ throw new IllegalArgumentException("Unknown Beam write property: " +
name);
Review Comment:
Would it be preferred to warn here (for future forward and backward
compatibility)
##########
sdks/java/extensions/sql/iceberg/src/main/java/org/apache/beam/sdk/extensions/sql/meta/provider/iceberg/IcebergTable.java:
##########
@@ -71,9 +74,21 @@ class IcebergTable extends SchemaBaseBeamTable {
this.tableIdentifier = tableIdentifier;
this.catalogConfig = catalogConfig;
ObjectNode properties = table.getProperties();
- if (properties.has(TRIGGERING_FREQUENCY_FIELD)) {
- this.triggeringFrequency =
properties.get(TRIGGERING_FREQUENCY_FIELD).asInt();
+ for (Map.Entry<String, JsonNode> property : properties.properties()) {
+ String name = property.getKey();
+ if (name.startsWith(BEAM_WRITE_PROPERTY)) {
Review Comment:
sql syntax is generally case insensitive. Would it also apply to here?
##########
website/www/site/assets/js/language-switch-v2.js:
##########
@@ -287,6 +287,7 @@ $(document).ready(function() {
}).render();
Switcher({"name": "runner", "default": "direct"}).render();
+ Switcher({"name": "tab"}).render();
Review Comment:
Put this line in order
##########
sdks/java/io/iceberg/src/main/java/org/apache/beam/sdk/io/iceberg/IcebergCatalogConfig.java:
##########
@@ -162,7 +162,11 @@ public void createTable(
icebergIdentifier,
icebergSchema,
icebergSpec);
- catalog().createTable(icebergIdentifier, icebergSchema, icebergSpec,
properties);
+ if (properties != null) {
Review Comment:
Is this necessary? In Iceberg the second overload method just append a null
```
default Table createTable(TableIdentifier identifier, Schema schema,
PartitionSpec spec) {
return createTable(identifier, schema, spec, null, null);
}
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
