-
Notifications
You must be signed in to change notification settings - Fork 12
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
Adds case insensitive decorators. #59
Conversation
...va/com/amazon/ionhiveserde/caseinsensitivedecorator/IonSequenceCaseInsensitiveDecorator.java
Outdated
Show resolved
Hide resolved
...va/com/amazon/ionhiveserde/caseinsensitivedecorator/IonSequenceCaseInsensitiveDecorator.java
Outdated
Show resolved
Hide resolved
...va/com/amazon/ionhiveserde/caseinsensitivedecorator/IonSequenceCaseInsensitiveDecorator.java
Outdated
Show resolved
Hide resolved
serde/src/test/kotlin/com/amazon/ionhiveserde/configuration/CaseInsensitiveDecoratorTest.kt
Outdated
Show resolved
Hide resolved
serde/src/test/kotlin/com/amazon/ionhiveserde/configuration/CaseInsensitiveDecoratorTest.kt
Outdated
Show resolved
Hide resolved
serde/src/test/kotlin/com/amazon/ionhiveserde/configuration/CaseInsensitiveDecoratorTest.kt
Show resolved
Hide resolved
serde/src/test/kotlin/com/amazon/ionhiveserde/configuration/CaseInsensitiveDecoratorTest.kt
Outdated
Show resolved
Hide resolved
serde/src/test/kotlin/com/amazon/ionhiveserde/configuration/CaseInsensitiveDecoratorTest.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! It's too bad that it's a pain to upgrade to Kotlin 1.5 right now, we're missing those assertions.
I also wonder whether it would be possible to somehow use Kotlin in our implementation and not just in test - it would make forwarding/decorator classes so much easier to implement.
Description
This PR fixs the issue #51. Ion is case sensitive and path extractor's case insensitive flag is targeting to the field names only (E.g. it doesn't affect fields within a nested struct).
Discussed with jobarr-amzn, one of the most scalable ways to solve the issue is implementing a case insensitive decorator which wraps an Ion Container. For read methods such as
containsKey
,get
,iterator
, the decorator should return a case insensitive decorator wrapped Ion Value as well without changing the underlying Ion struct to make sure all fields within a nested container are still case insensitive.Reproduce the issue (solved in this PR):
Ion text:
{foo: {Bar: 1}}
Hive QL:
Reading from Hive (Before):
It shows (Now):
Root:
The root of this issue is because ion-java-path-extractor will call the callback method directly and return the IonStruct without stepping into it (because it was no need to spend extra time to step-in the struct) when ion-java-path-extractor matches the desired search path. As a result, cases of filed names within a struct are never changed (e.g. they are still uppercase).
Test:
Passed unit tests/integration tests. In addition, it worked correctly for #51 example in the EMR environment.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.