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

EQL: code cleanup and further tests #58458

Merged
merged 4 commits into from
Jun 24, 2020
Merged

Conversation

astefan
Copy link
Contributor

@astefan astefan commented Jun 23, 2020

  • adds Pipe tests to all function
  • missing Processor tests for string and substring functions
  • use an overall input named parameter to avoid confusion with the already existent source, src in all functions related classes
  • other minor bug-fixes and code cleanups

A wide NodeSubclass test class to be added later with #53862.

Addresses #54568.

@astefan astefan added >enhancement >test Issues or PRs that are addressing/adding tests :Analytics/EQL EQL querying labels Jun 23, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-ql (:Query Languages/EQL)

@elasticmachine elasticmachine added the Team:QL (Deprecated) Meta label for query languages team label Jun 23, 2020
@astefan astefan requested review from costin and matriv June 24, 2020 05:30
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. Nice work adding all those missing tests!
Left a couple of minor questions.

@@ -85,6 +85,10 @@ public ConcatFunctionProcessor asProcessor() {
return new ConcatFunctionProcessor(processors);
}

public List<Pipe> values() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is public necessary? can it be package private?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, can be package private.

@@ -559,7 +559,7 @@ private Object makeMap(Class<? extends Node<?>> toBuildClass, ParameterizedType

private int randomSizeForCollection(Class<? extends Node<?>> toBuildClass) {
int minCollectionLength = 0;
int maxCollectionLength = 10;
int maxCollectionLength = 3;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you reduce this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. That's a leftover from an attempt to add EqlNodeSubclassTests to EQL. Will revert it.

Copy link
Member

@costin costin left a comment

Choose a reason for hiding this comment

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

LGTM!

@astefan astefan merged commit 0f83d57 into elastic:master Jun 24, 2020
@astefan astefan deleted the 54568_impl branch June 24, 2020 13:24
astefan added a commit to astefan/elasticsearch that referenced this pull request Jun 24, 2020
Add FunctionPipe tests to all functions. Cleanup functions code.

(cherry picked from commit 0f83d57)
astefan added a commit that referenced this pull request Jun 24, 2020
Add FunctionPipe tests to all functions. Cleanup functions code.

(cherry picked from commit 0f83d57)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/EQL EQL querying >refactoring Team:QL (Deprecated) Meta label for query languages team >test Issues or PRs that are addressing/adding tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants