You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "wombatu-kun (via GitHub)" <gi...@apache.org> on 2026/06/02 04:16:25 UTC
[PR] Kafka Connect: Split route field path once instead of per record [iceberg]
wombatu-kun opened a new pull request, #16655:
URL: https://github.com/apache/iceberg/pull/16655
When routing by a field (static routing with `iceberg.tables.route-field`, or dynamic routing), `SinkWriter` extracted the route value for every record via `RecordUtils.extractFromRecordValue(value, routeField)`, which re-parsed the dotted field path with `Splitter.on('.').splitToList(routeField)` on each call. The route field is fixed for the connector's lifetime, so this re-parse is pure per-record overhead.
This splits the path once in the `SinkWriter` constructor and adds a `RecordUtils.extractFromRecordValue(Object, List<String>)` overload that takes the already-split path; the existing `String` overload now delegates to it, so other callers are unchanged. Behavior is identical.
A throwaway A/B microbench over the whole `extractFromRecordValue` method (2M iterations x 9 trials, median; baseline = current `String` overload that splits per call, optimized = `List` overload with the path split once) showed:
| record value | route field | before | after | faster |
|---|---|---|---|---|
| struct | `key` | 52.8 ns | 5.7 ns | 89% |
| struct | `data.id.key` | 162.2 ns | 32.0 ns | 80% |
| map | `key` | 54.3 ns | 6.1 ns | 89% |
| map | `data.id.key` | 144.3 ns | 21.6 ns | 85% |
That is roughly 47 ns saved per record for a single-segment route field and ~120-130 ns for a three-segment path, paid once per record on the routing path. The numbers are indicative wall-clock from a microbench, not JMH.
Existing `TestSinkWriter` and `TestRecordUtils` cover both routing modes and the extraction overloads.
--
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] Kafka Connect: Split route field path once instead of per record [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #16655:
URL: https://github.com/apache/iceberg/pull/16655#issuecomment-6103786808
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.
--
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] Kafka Connect: Split route field path once instead of per record [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #16655:
URL: https://github.com/apache/iceberg/pull/16655#issuecomment-5234610574
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.
--
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] Kafka Connect: Split route field path once instead of per record [iceberg]
Posted by "wombatu-kun (via GitHub)" <gi...@apache.org>.
wombatu-kun commented on code in PR #16655:
URL: https://github.com/apache/iceberg/pull/16655#discussion_r4244688861
##########
kafka-connect/kafka-connect/src/main/java/org/apache/iceberg/connect/data/RecordUtils.java:
##########
@@ -47,9 +47,12 @@
class RecordUtils {
- @SuppressWarnings("unchecked")
static Object extractFromRecordValue(Object recordValue, String fieldName) {
- List<String> fields = Splitter.on('.').splitToList(fieldName);
+ return extractFromRecordValue(recordValue, Splitter.on('.').splitToList(fieldName));
+ }
+
+ @SuppressWarnings("unchecked")
+ static Object extractFromRecordValue(Object recordValue, List<String> fields) {
Review Comment:
Dropped the `String` overload in 23e1b0073a, and `TestRecordUtils` now splits with `Splitter.on('.').splitToList(...)`, so it exercises the production signature. Since the split now happens in the `SinkWriter` constructor, I also added `TestSinkWriter.dynamicRouteNestedField`, which routes on a nested `route.table` field and fails if the constructor stops splitting.
--
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] Kafka Connect: Split route field path once instead of per record [iceberg]
Posted by "wombatu-kun (via GitHub)" <gi...@apache.org>.
wombatu-kun commented on PR #16655:
URL: https://github.com/apache/iceberg/pull/16655#issuecomment-6117826076
@laskoviymishka both of your comments are addressed in 23e1b0073a (replies inline), and CI is green. Could you merge it when you have a moment? Thanks!
--
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] Kafka Connect: Split route field path once instead of per record [iceberg]
Posted by "wombatu-kun (via GitHub)" <gi...@apache.org>.
wombatu-kun commented on PR #16655:
URL: https://github.com/apache/iceberg/pull/16655#issuecomment-5251599846
@laskoviymishka you merged #16657 and #16606 in this same `connect/data` package - would you have a moment for this one? Same shape as #16657: per-record work hoisted out of the hot path, 2 files, CI green.
--
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] Kafka Connect: Split route field path once instead of per record [iceberg]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka commented on code in PR #16655:
URL: https://github.com/apache/iceberg/pull/16655#discussion_r4242885691
##########
kafka-connect/kafka-connect/src/main/java/org/apache/iceberg/connect/data/SinkWriter.java:
##########
@@ -39,12 +40,16 @@ public class SinkWriter {
private final IcebergWriterFactory writerFactory;
private final Map<String, RecordWriter> writers;
private final Map<TopicPartition, Offset> sourceOffsets;
+ // the route field is fixed per config, so split its dotted path once instead of per record
Review Comment:
I'd drop this comment — it describes what the PR changed ("instead of per record"), which stops making sense once the old per-record split is gone, and the field name already says what it holds. If we want a comment here, the one non-obvious bit is the null contract: `routeFieldPath == null` means route to all tables.
##########
kafka-connect/kafka-connect/src/main/java/org/apache/iceberg/connect/data/RecordUtils.java:
##########
@@ -47,9 +47,12 @@
class RecordUtils {
- @SuppressWarnings("unchecked")
static Object extractFromRecordValue(Object recordValue, String fieldName) {
- List<String> fields = Splitter.on('.').splitToList(fieldName);
+ return extractFromRecordValue(recordValue, Splitter.on('.').splitToList(fieldName));
+ }
+
+ @SuppressWarnings("unchecked")
+ static Object extractFromRecordValue(Object recordValue, List<String> fields) {
Review Comment:
After this change the only production caller goes through the `List<String>` overload, so the `String` version is left serving just `TestRecordUtils`. Either is fine, but I'd pick deliberately: drop the `String` overload and have the tests split via `Splitter.on('.').splitToList(...)` so they exercise the production signature, or keep it and treat it as the covered split-then-extract convenience. It's package-private, so there's no API cost either way.
--
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] Kafka Connect: Split route field path once instead of per record [iceberg]
Posted by "wombatu-kun (via GitHub)" <gi...@apache.org>.
wombatu-kun commented on code in PR #16655:
URL: https://github.com/apache/iceberg/pull/16655#discussion_r4244687075
##########
kafka-connect/kafka-connect/src/main/java/org/apache/iceberg/connect/data/SinkWriter.java:
##########
@@ -39,12 +40,16 @@ public class SinkWriter {
private final IcebergWriterFactory writerFactory;
private final Map<String, RecordWriter> writers;
private final Map<TopicPartition, Offset> sourceOffsets;
+ // the route field is fixed per config, so split its dotted path once instead of per record
Review Comment:
Dropped in 23e1b0073a, without a replacement: the null contract is already stated where the field is read, by the `// route to all tables` branch on the static path and the `checkNotNull` message on the dynamic one.
--
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] Kafka Connect: Split route field path once instead of per record [iceberg]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka commented on PR #16655:
URL: https://github.com/apache/iceberg/pull/16655#issuecomment-6121919145
Thanks! This is good to merge now
--
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] Kafka Connect: Split route field path once instead of per record [iceberg]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka merged PR #16655:
URL: https://github.com/apache/iceberg/pull/16655
--
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