Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

KAFKA-16288, KAFKA-16289: Fix Values convertToDecimal exception and parseString corruption #15399

Merged

Conversation

gharris1727
Copy link
Contributor

See the descriptions of the tickets for full details:

These both represent breaking changes in behavior, but only when using incompatible-type arrays and maps, such as the ones included in the tests. Since the behavior of these is so opaque and silent corruption is possible with the bugs, we should change the behavior unconditionally.

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

…tToDecimal

Signed-off-by: Greg Harris <greg.harris@aiven.io>
… element order

Signed-off-by: Greg Harris <greg.harris@aiven.io>
Comment on lines +810 to +811
assertEquals(Type.ARRAY, schema.type());
assertNull(schema.valueSchema());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it too difficult to also add an assertion about the parsed values in the array? Not a blocker, but seems nice to have if possible, especially since we don't cover anything except various representations of 1 in the other test above.

Copy link
Contributor

@C0urante C0urante left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks Greg!

@gharris1727
Copy link
Contributor Author

Test failures appear unrelated, and the tests pass for me locally.

@gharris1727 gharris1727 merged commit 99e511c into apache:trunk Mar 4, 2024
1 check failed
testn pushed a commit to testn/kafka that referenced this pull request Mar 15, 2024
…arseString corruption (apache#15399)

* KAFKA-16288: Prevent ClassCastExceptions for strings in Values.convertToDecimal
* KAFKA-16289: Values inferred schemas for map and arrays should ignore element order

Signed-off-by: Greg Harris <greg.harris@aiven.io>
Reviewers: Chris Egerton <chrise@aiven.io>
clolov pushed a commit to clolov/kafka that referenced this pull request Apr 5, 2024
…arseString corruption (apache#15399)

* KAFKA-16288: Prevent ClassCastExceptions for strings in Values.convertToDecimal
* KAFKA-16289: Values inferred schemas for map and arrays should ignore element order

Signed-off-by: Greg Harris <greg.harris@aiven.io>
Reviewers: Chris Egerton <chrise@aiven.io>
Phuc-Hong-Tran pushed a commit to Phuc-Hong-Tran/kafka that referenced this pull request Jun 6, 2024
…arseString corruption (apache#15399)

* KAFKA-16288: Prevent ClassCastExceptions for strings in Values.convertToDecimal
* KAFKA-16289: Values inferred schemas for map and arrays should ignore element order

Signed-off-by: Greg Harris <greg.harris@aiven.io>
Reviewers: Chris Egerton <chrise@aiven.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants