Skip to content

fix bugs with nested column jsonpath parser#12831

Merged
clintropolis merged 1 commit intoapache:masterfrom
clintropolis:json-column-fix-jsonpath-parser
Aug 2, 2022
Merged

fix bugs with nested column jsonpath parser#12831
clintropolis merged 1 commit intoapache:masterfrom
clintropolis:json-column-fix-jsonpath-parser

Conversation

@clintropolis
Copy link
Member

Description

Fixes some bugs with the custom jsonpath parser used by nested columns. It was overly restrictive in where ' could appear, which prevented extracting properties from json that contain single quotes, as well as fixes some issues with the 'normalized' jsonpath generated from parsed path parts.

This PR has:

  • been self-reviewed.
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
  • been tested in a test Druid cluster.

Copy link
Contributor

@imply-cheddar imply-cheddar left a comment

Choose a reason for hiding this comment

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

I don't see new tests added, only changing of old tests. Was the bad behavior actually captured and encoded in those tests such that changing them is now verifying good behavior? Is there really no new thing that we can add to also test the bug fix here?

@clintropolis
Copy link
Member Author

clintropolis commented Jul 30, 2022

I don't see new tests added, only changing of old tests. Was the bad behavior actually captured and encoded in those tests such that changing them is now verifying good behavior? Is there really no new thing that we can add to also test the bug fix here?

The "new" test is https://github.com/apache/druid/pull/12831/files#diff-886d639244f5abe97ecb6fcfed49b7f6baaa041b10d843982e82984c9f5653a4R316 which was just added another case to a test that tests a variety of paths. The case immediately before it was a case of the incorrect path being "expected" and so fixed.

@clintropolis clintropolis merged commit 6981b1c into apache:master Aug 2, 2022
@clintropolis clintropolis deleted the json-column-fix-jsonpath-parser branch August 2, 2022 18:38
@abhishekagarwal87 abhishekagarwal87 added this to the 24.0.0 milestone Aug 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants