Skip to content

fix issue with boolean expression input#12429

Merged
clintropolis merged 2 commits intoapache:masterfrom
clintropolis:fix-boolean-eval
Apr 13, 2022
Merged

fix issue with boolean expression input#12429
clintropolis merged 2 commits intoapache:masterfrom
clintropolis:fix-boolean-eval

Conversation

@clintropolis
Copy link
Copy Markdown
Member

Description

Fixes a regression caused by #11853 when processing boolean typed inputs for transformation expressions at ingestion time, such as from JSON and other input formats which support them. The changes to how expression types were resolved to make room for COMPLEX, resulted in expression input that is of the Boolean java type ending up as COMPLEX druid type. Now, if these values will be correctly converted to either STRING or LONG druid types, if druid.expressions.useStrictBooleans=true.


This PR has:

  • been self-reviewed.
  • 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
Copy Markdown
Contributor

@jihoonson jihoonson left a comment

Choose a reason for hiding this comment

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

LGTM overall. One question is, what should ofType do when type is DOUBLE and value is a boolean? Even though it doesn't seem to be able to happen today, I think it would be nicer if the ofType method is more complete and handles every case. I guess the method should either fail or handles the input properly.

Copy link
Copy Markdown
Contributor

@jihoonson jihoonson 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 @clintropolis.

@clintropolis clintropolis merged commit 5824ab9 into apache:master Apr 13, 2022
@clintropolis clintropolis deleted the fix-boolean-eval branch April 13, 2022 23:34
@abhishekagarwal87 abhishekagarwal87 added this to the 0.23.0 milestone May 11, 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.

3 participants