Skip to content

SQL: ConstantProcessor can now handle NamedWriteable #39876

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

Merged
merged 1 commit into from
Mar 11, 2019

Conversation

costin
Copy link
Member

@costin costin commented Mar 9, 2019

Enhance ConstantProcessor to properly serialize complex objects
(Intervals) that have their own custom serialization/deserialization
mechanism

Fix #39875

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search

@costin costin requested review from astefan and matriv March 9, 2019 10:10
Enhance ConstantProcessor to properly serialize complex objects
(Intervals) that have their own custom serialization/deserialization
mechanism

Fix elastic#39875

Update test
@costin
Copy link
Member Author

costin commented Mar 9, 2019

run elasticsearch-ci/packaging-sample

Copy link
Contributor

@matriv matriv left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -28,7 +31,10 @@ protected ConstantProcessor createTestInstance() {

@Override
protected ConstantProcessor mutateInstance(ConstantProcessor instance) throws IOException {
return new ConstantProcessor(randomValueOtherThan(instance.process(null), () -> randomAlphaOfLength(5)));
return new ConstantProcessor(randomValueOtherThan(instance.process(null),
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not also keeping the original randomAlphaOfLength? Using a randomBoolean you could switch between randomAlphaOfLength and an IntervalDayTime instance.

Copy link
Member Author

Choose a reason for hiding this comment

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

Because the randomAlpha is already tested from the initial instance which gets serialized/deserialized so using it again wouldn't change much.

Copy link
Contributor

@astefan astefan left a comment

Choose a reason for hiding this comment

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

LGTM. Left one tests related question.

@costin costin merged commit ed8a1f9 into elastic:master Mar 11, 2019
@costin costin deleted the fix-39875 branch March 11, 2019 09:50
costin added a commit that referenced this pull request Mar 11, 2019
Enhance ConstantProcessor to properly serialize complex objects
(Intervals) that have their own custom serialization/deserialization
mechanism

Fix #39875
costin added a commit that referenced this pull request Mar 11, 2019
Enhance ConstantProcessor to properly serialize complex objects
(Intervals) that have their own custom serialization/deserialization
mechanism

Fix #39875

(cherry picked from commit ed8a1f9)
costin added a commit that referenced this pull request Mar 11, 2019
Enhance ConstantProcessor to properly serialize complex objects
(Intervals) that have their own custom serialization/deserialization
mechanism

Fix #39875

(cherry picked from commit ed8a1f9)
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.

SQL: ConstantProcessor cannot handle Interval values
5 participants