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

Add ES|QL signum function #106866

Merged
merged 7 commits into from
Apr 4, 2024
Merged

Add ES|QL signum function #106866

merged 7 commits into from
Apr 4, 2024

Conversation

ioanatia
Copy link
Contributor

#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
    ]
  ]
}

Copy link
Contributor

Documentation preview:

@elasticsearchmachine
Copy link
Collaborator

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

@ioanatia ioanatia mentioned this pull request Mar 28, 2024
76 tasks
@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
Collaborator

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

-100 | -1.0
;

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

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
Contributor Author

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
Member

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.

@@ -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
Member

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
Member

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
Contributor Author

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!

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.

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
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
14 checks passed
@ioanatia
Copy link
Contributor 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
Labels
:Analytics/ES|QL AKA ESQL >enhancement Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v8.14.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants