Repository navigation
Conversation
…lumnPrefix KafkaInputFormat only kept the first field of a parsed Kafka key. Setting the new optional keyColumnPrefix property makes the reader keep every field of the parsed key (after any flattenSpec on keyFormat) as a column named keyColumnPrefix + fieldName, in both read() and sample(). Default behavior and serialized specs are unchanged when the property is not set. Fixes apache#18578. Generated-by: Claude (AI coding assistant), reviewed and tested by the author
FrankChen021
left a comment
There was a problem hiding this comment.
🟡 Changes recommended
Preserve the existing public constructor signatures before merging. Adding the optional prefix currently breaks direct Java or custom-extension callers even when keyColumnPrefix is unset.
Reviewed 7 of 7 changed files; findings in 1 file.
Validation: git diff --no-ext-diff --check 7e300b9ccf6cca26d7bbe632f790415b31f123c1 d2ff79db70f2b5c398a10247e23ee6cf77b2094b passed. No build or tests were run.
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 0 |
| P2 | 1 |
| P3 | 0 |
| Total | 1 |
This is an automated review by Codex GPT-5.6-Luna(max)
After addressing the findings or replying to the comments, you can request another review from me to trigger a new automated review.
| @JsonProperty("partitionColumnName") @Nullable String partitionColumnName, | ||
| @JsonProperty("offsetColumnName") @Nullable String offsetColumnName | ||
| @JsonProperty("offsetColumnName") @Nullable String offsetColumnName, | ||
| @JsonProperty("keyColumnPrefix") @Nullable String keyColumnPrefix |
There was a problem hiding this comment.
[P2] Preserve the existing public constructor
Finding: Appending keyColumnPrefix removes the previous public KafkaInputFormat constructor signature, so Java callers compiled against the prior release can fail with NoSuchMethodError or fail to compile even when they do not use the new option. The corresponding public KafkaInputReader constructor is changed in the same way.
Suggestion: Retain overloads for the previous KafkaInputFormat and KafkaInputReader signatures, delegating to the new constructors with a null prefix.
There was a problem hiding this comment.
can ignore this finding from AI
Fixes #18578.
Description
KafkaInputFormatparses the Kafka key withkeyFormat, but only keeps the first field of the parsed key (askeyColumnName). Topics with structured keys, such as a JSON or Avro key with a tenant id and an entity id, lose every field after the first one, so users have to duplicate key fields into the payload or pre-process the topic.This adds an optional
keyColumnPrefixproperty to thekafkainput format. When it is set, every field of the parsed key is ingested as its own column, namedkeyColumnPrefix + fieldName:A key of
{"tenant": "acme", "userId": 42}becomes the columnskafka.key.tenantandkafka.key.userId.Behavior details:
keyColumnName. Specs that don't set the property serialize exactly as before (@JsonInclude(NON_NULL)).flattenSpeconkeyFormatis applied first, so flattened fields are included.timestampSpeccolumn.sample()returns the same key columns asread()(it uses the parsed key row, so flattened fields show up in the sampler too).Alternative considered: changing the default to ingest all key fields. I kept the default as is because it would change the schema of existing datasources on upgrade, and an empty prefix (
"") already allows unprefixed key columns for users who want them.Release note
The Kafka input format has a new optional
keyColumnPrefixproperty. When set, Druid ingests every field of a structured Kafka key (for example a multi-field JSON or Avro key) as its own column instead of only the first field.Key changed/added classes in this PR
KafkaInputFormatKafkaInputReaderThis PR has:
Testing:
mvn -pl extensions-core/kafka-indexing-service test(checkstyle and forbidden-apis pass).KafkaInputFormatTesthas 9 new tests: serde, multi-field JSON key, keyflattenSpec, CSV key, timestamp from a prefixed key field, precedence, null key, and sampling with and without a flattenSpec. The Testcontainers-based tests in the module (KafkaIndexTaskTest,KafkaSamplerSpecTest, the embedded supervisor tests) need Docker, which I don't have locally, so those rely on CI. The web console doesn't expose the new field yet; I can follow up with that if this approach looks right.This change was written with help from an AI coding assistant; I reviewed the code and ran the tests above.