You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "bryanck (via GitHub)" <gi...@apache.org> on 2025/02/11 16:22:13 UTC
[PR] Core: Adjust Jackson settings to handle large metadata json [iceberg]
bryanck opened a new pull request, #12224:
URL: https://github.com/apache/iceberg/pull/12224
With very large table metadata json, for example, those with many snapshots with partition summaries, we sometimes encounter errors involving hash collisions when loading the metadata. This PR disables that hash collision check so the metadata can be parsed without error. We have had this set in our internal fork for a while.
In addition, this PR disable string interning of field names which has lead to performance problems for us when parsing metadata. Given partition summary field names and other snapshot properties are often not reused across different metadata, the interning causes more harm than good. This is especially true when using Iceberg in a server which is loading metadata for many tables.
--
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] Core: Adjust Jackson settings to handle large metadata json [iceberg]
Posted by "stevenzwu (via GitHub)" <gi...@apache.org>.
stevenzwu commented on PR #12224:
URL: https://github.com/apache/iceberg/pull/12224#issuecomment-2651821329
> Canonicalization can help when field names are reused within a single metadata file, so that seemed helpful still.
canonicalization lifecycle is scoped to a single metadata file? if it is also JVM lifecycle scope (like String intern), it can also be a problem for large tables and a server handling many tables.
--
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] Core: Adjust Jackson settings to handle large metadata json [iceberg]
Posted by "bryanck (via GitHub)" <gi...@apache.org>.
bryanck commented on PR #12224:
URL: https://github.com/apache/iceberg/pull/12224#issuecomment-2651697343
> @bryanck I didn't quite get the partition summary field names. were you referring to `PartitionFieldSummaryParser`? it seems to have just 4 field names.
>
> String.intern can be helpful for some use cases while harmful for some (like the one you encountered). Disabling interning seems to be a safer option considering diverse scenarios that the code can be used (like REST catalog server).
The information for each partition key has a field name unique to the partition (with the prefix `partition.`). There is some discussion around intern [here](https://github.com/FasterXML/jackson-core/issues/332) with more links. TL;DR is that intern was disabled by default for Jackson 3 (whenever that is released).
> I definitely understand the situation you described. maybe reach out to the Jackson authors too according to the doc? https://github.com/fasterxml/jackson-core/wiki/JsonFactory-Features
Sure sounds good, I'll reach out.
> The doc also mentioned that hash collision check is `Only relevant if canonicalization is enabled`. wondering if `CANONICALIZE_FIELD_NAMES` should be disabled too. I imagined it can cause similar memory footprint issue as String interning.
Canonicalization can help when field names are reused within a single metadata file, so that seemed helpful still.
--
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] Core: Adjust Jackson settings to handle large metadata json [iceberg]
Posted by "bryanck (via GitHub)" <gi...@apache.org>.
bryanck commented on PR #12224:
URL: https://github.com/apache/iceberg/pull/12224#issuecomment-2652018035
I switched back to the original change, to just disable intern and the hash collision check. Disabling canonicalization can impact performance significantly.
--
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] Core: Adjust Jackson settings to handle large metadata json [iceberg]
Posted by "bryanck (via GitHub)" <gi...@apache.org>.
bryanck commented on PR #12224:
URL: https://github.com/apache/iceberg/pull/12224#issuecomment-2651840830
> > > Canonicalization can help when field names are reused within a single metadata file, so that seemed helpful still.
> >
> >
> > canonicalization lifecycle is scoped to a single metadata file? if it is also JVM lifecycle scope (like String intern), it can also be a problem for large tables and a server handling many tables.
>
> I believe it is scoped to a parser instance, and we generally create a new parser for each AFAIK. (https://github.com/fasterxml/jackson-core/wiki/JsonFactory-Features)
Actually that doesn't seem correct, it is for any parser created by the same factory, so we should probably turn canonicalization off 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] Core: Adjust Jackson settings to handle large metadata json [iceberg]
Posted by "bryanck (via GitHub)" <gi...@apache.org>.
bryanck commented on PR #12224:
URL: https://github.com/apache/iceberg/pull/12224#issuecomment-2652364540
> [for my understanding] I thought we had a way to lazy load metadata in REST, the complete metadata parsing would only be required at the time of commit ? Are all the tables write heavy ?
We have a very high write load and generally have partition summaries turned on.
--
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] Core: Adjust Jackson settings to handle large metadata json [iceberg]
Posted by "stevenzwu (via GitHub)" <gi...@apache.org>.
stevenzwu commented on PR #12224:
URL: https://github.com/apache/iceberg/pull/12224#issuecomment-2651667891
> Given partition summary field names and other snapshot properties are often not reused across different metadata, the interning causes more harm than good.
@bryanck I didn't quite get the partition summary field names. were you referring to `PartitionFieldSummaryParser`? it seems to have just 4 field names.
String.intern can be helpful for some use cases while harmful for some (like the one you encountered). Disabling interning seems to be a safer option considering diverse scenarios that the code can be used (like REST catalog server).
> hash collision check
I definitely understand the situation you described. maybe reach out to the Jackson authors too according to the doc?
https://github.com/fasterxml/jackson-core/wiki/JsonFactory-Features
```
In unlikely event that the exception is triggered for valid data, it may make sense to either disable this feature, or to disable canonicalization. However, Jackson authors would also like to be notified for such usage as it may point to an issue with hashing scheme -- so please file an issue if you encounter this problem.
```
The doc also mentioned that hash collision check is `Only relevant if canonicalization is enabled`. wondering if `CANONICALIZE_FIELD_NAMES` should be disabled too. I imagined it can cause similar memory footprint issue as String interning.
--
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] Core: Adjust Jackson settings to handle large metadata json [iceberg]
Posted by "bryanck (via GitHub)" <gi...@apache.org>.
bryanck commented on PR #12224:
URL: https://github.com/apache/iceberg/pull/12224#issuecomment-2651863061
We just need to disable `FAIL_ON_SYMBOL_HASH_OVERFLOW`, that will also disable canonicalization, according to the docs. I made that 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
Re: [PR] Core: Adjust Jackson settings to handle large metadata json [iceberg]
Posted by "bryanck (via GitHub)" <gi...@apache.org>.
bryanck commented on PR #12224:
URL: https://github.com/apache/iceberg/pull/12224#issuecomment-2651836612
> > Canonicalization can help when field names are reused within a single metadata file, so that seemed helpful still.
>
> canonicalization lifecycle is scoped to a single metadata file? if it is also JVM lifecycle scope (like String intern), it can also be a problem for large tables and a server handling many tables.
I believe it is scoped to a parser instance, and we generally create a new parser for each AFAIK. (https://github.com/fasterxml/jackson-core/wiki/JsonFactory-Features)
--
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] Core: Adjust Jackson settings to handle large metadata json [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra merged PR #12224:
URL: https://github.com/apache/iceberg/pull/12224
--
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] Core: Adjust Jackson settings to handle large metadata json [iceberg]
Posted by "stevenzwu (via GitHub)" <gi...@apache.org>.
stevenzwu commented on PR #12224:
URL: https://github.com/apache/iceberg/pull/12224#issuecomment-2652222384
@bryanck thanks for the experimentation with canonicalization. do you have any micro/jmh benchmark for the parser performance? if yes, maybe it would be useful to add it to the Iceberg repo.
--
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