amogh-jahagirdar commented on code in PR #8723:
URL: https://github.com/apache/iceberg/pull/8723#discussion_r1347395043
##########
core/src/main/java/org/apache/iceberg/view/ViewMetadataParser.java:
##########
@@ -66,7 +67,9 @@ static void toJson(ViewMetadata metadata, JsonGenerator gen)
throws IOException
gen.writeStringField(VIEW_UUID, metadata.uuid());
gen.writeNumberField(FORMAT_VERSION, metadata.formatVersion());
gen.writeStringField(LOCATION, metadata.location());
- JsonUtil.writeStringMap(PROPERTIES, metadata.properties(), gen);
+ if (!metadata.properties().isEmpty()) {
Review Comment:
@ajantha-bhat Right, my point is I think what TableMetadata is doing already
is fine and we should be able to follow the same idea here.
In other words, why make it optional in the first place for views on the
write side when going through the Iceberg provided metadata writer?
I can see the argument for optional reads since there's no point failing
reading view metadata if it's missing properties, in case somebody messed with
metadata or has their own metadata writer (so the else block mentioned is not
really dead code in this case)
. That's OK to me because properties are optional from a spec perspective. I
think that on the write side the Iceberg library metadata writer should just
write out the properties.
--
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
