Skip to content

Add ES|QL signum function#106866

Merged
ioanatia merged 7 commits intoelastic:mainfrom
ioanatia:esql_signum
Apr 4, 2024
Merged

Add ES|QL signum function#106866
ioanatia merged 7 commits intoelastic:mainfrom
ioanatia:esql_signum

Conversation

@ioanatia
Copy link
Copy Markdown
Member

@ioanatia ioanatia commented Mar 28, 2024

#98545

Implements the signum function.

PUT testidx
{
  "mappings": {
    "properties": {
      "float_field": {
        "type": "float"
      }
    }
  }
}

POST testidx/_doc
{ "float_field": 25 }

POST testidx/_doc
{ "float_field": -100.0 }

POST testidx/_doc
{ "float_field": 0 }

POST testidx/_doc
{ "float_field": null }


POST _query
{
  "query": """
  FROM testidx|
  EVAL s = signum(float_field)
  """
}

result:

{
  "columns": [
    {
      "name": "float_field",
      "type": "double"
    },
    {
      "name": "s",
      "type": "double"
    }
  ],
  "values": [
    [
      25,
      1
    ],
    [
      -100,
      -1
    ],
    [
      0,
      0
    ],
    [
      null,
      null
    ]
  ]
}

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 28, 2024

Documentation preview:

@elasticsearchmachine
Copy link
Copy Markdown
Collaborator

elasticsearchmachine commented Mar 28, 2024

Hi @ioanatia, I've created a changelog YAML for you.

@ioanatia ioanatia mentioned this pull request Mar 28, 2024
Open
@ioanatia ioanatia marked this pull request as ready for review March 28, 2024 13:02
@elasticsearchmachine elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Mar 28, 2024
@elasticsearchmachine
Copy link
Copy Markdown
Collaborator

elasticsearchmachine commented Mar 28, 2024

Pinging @elastic/es-analytical-engine (Team:Analytics)

nik9000
nik9000 approved these changes Mar 28, 2024
View reviewed changes
x-pack/plugin/esql/qa/testFixtures/src/main/resources/math.csv-spec Outdated
-100 | -1.0
;

signumOfZeroInteger#[skip:-8.13.99,reason:new scalar function added in 8.14]
Copy link
Copy Markdown
Member

@nik9000 nik9000 Mar 28, 2024

Choose a reason for hiding this comment

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

I've been leaning towards putting these in floats.csv-spec and ints.csv-spec and unsigned_long.csv-spec.

Copy link
Copy Markdown
Member Author

@ioanatia ioanatia Mar 28, 2024

Choose a reason for hiding this comment

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

Done - and in this case, I also don't need to add #[skip:-8.13.99,reason:new scalar function added in 8.14]?
doesn't seem like it from the other specs in these files.

Copy link
Copy Markdown
Member

@nik9000 nik9000 Apr 3, 2024

Choose a reason for hiding this comment

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

If it's new I believe you still need it. I believe we'll be transitioning these to the "features" APIs before too long, but that's half way through at the moment so I'd stick with the skips.

You have to run something like ./gradlew -p x-pack/plugin/esql/qa/server/mixed-cluster v8.13.1#bwcTest to bump into it.

docs/reference/esql/functions/layout/signum.asciidoc
@@ -0,0 +1,15 @@
// This is generated by ESQL's AbstractFunctionTestCase. Do no edit it. See ../README.md for how to regenerate it.
Copy link
Copy Markdown
Member

@nik9000 nik9000 Mar 28, 2024

Choose a reason for hiding this comment

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

If you link to this file in math-functions.asciidoc then we'll render the generated docs.

Copy link
Copy Markdown
Member

@nik9000 nik9000 Mar 28, 2024

Choose a reason for hiding this comment

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

Would you mind updating the package-info.java file with instructions to do this? I should have done it when I wrote the code generation, but I forgot.

Copy link
Copy Markdown
Member Author

@ioanatia ioanatia Mar 28, 2024

Choose a reason for hiding this comment

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

Done and done! the package-info.java instructions are great btw!

astefan
astefan previously requested changes Mar 28, 2024
View reviewed changes
Copy link
Copy Markdown
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.

Thank you for your contribution.

Having csv-spec tests that only use row is not enough.
Take a look at other csv-spec files where from command is used and try to have more complex queries with nested functions, using the signum function in all other commands (sort, eval, filter, stats etc).

@nik9000
Copy link
Copy Markdown
Member

nik9000 commented Apr 1, 2024

Having csv-spec tests that only use row is not enough.

Ah! I should have caught that one.

@ioanatia ioanatia requested review from astefan and nik9000 April 3, 2024 13:28
@ioanatia ioanatia dismissed astefan’s stale review April 4, 2024 07:48

the feedback has been addressed.

@ioanatia ioanatia merged commit 7b25421 into elastic:main Apr 4, 2024
@ioanatia
Copy link
Copy Markdown
Member Author

ioanatia commented Apr 4, 2024

@astefan - more tests have been added and Nik reviewed the PR again yesterday.
I merged it since I wanted to avoid any new merge conflicts, but if there is any additional feedback from you let me know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nik9000 nik9000 nik9000 approved these changes

@astefan astefan Awaiting requested review from astefan

Assignees

No one assigned

Labels

:Analytics/ES|QL AKA ESQL >enhancement Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v8.14.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ioanatia @elasticsearchmachine @nik9000 @astefan