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

[BUG] MIN(str, str) and MAX(str, str) functions fail #279

Open
Tracked by #817
Yury-Fridlyand opened this issue Nov 11, 2021 · 3 comments
Open
Tracked by #817

[BUG] MIN(str, str) and MAX(str, str) functions fail #279

Yury-Fridlyand opened this issue Nov 11, 2021 · 3 comments
Labels
bug Something isn't working powerBI SQL tdvt Tableau test framework

Comments

@Yury-Fridlyand
Copy link
Collaborator

Describe the bug
Queries with functions MIN and MAX called with 2 arguments of type string fail and produce incorrect response

To Reproduce

curl -XPOST http://localhost:9200/_plugins/_sql -H 'Content-Type: application/json' -d '{"query":  select MAX (\"Apple\",\"Banana\") from calcs limit 1;"}''
JSON does not allow non-finite numbers.
curl -XPOST http://localhost:9200/_plugins/_sql -H 'Content-Type: application/json' -d '{"query":  select MIN (\"Apple\",\"Banana\") from calcs limit 1;"}''
JSON does not allow non-finite numbers.

Expected behavior
According to Tableau's definition:

With strings, MAX finds the value that is highest in the sort sequence defined by the database for that column. It returns Null if either argument is Null.
Example
MAX ("Apple","Banana") = "Banana"

The server log is not verbose:

[2021-11-11T05:00:09,303][WARN ][o.o.s.l.u.QueryDataAnonymizer] [6a3601a40d1d]  Caught an exception when anonymizing sensitive data
[2021-11-11T05:00:09,303][INFO ][o.o.s.l.p.RestSqlAction  ] [6a3601a40d1d]  [bab60cbb-d175-4c45-806f-b05fc08bd5fe] Incoming request /_plugins/_sql:  select MAX ("Apple","Banana") from calcs limit 1;
[2021-11-11T05:00:09,313][WARN ][stderr                   ] [6a3601a40d1d]  line 1:20 no viable alternative at input 'MAX ("Apple",'
[2021-11-11T05:00:09,316][WARN ][o.o.s.l.e.AsyncRestExecutor] [6a3601a40d1d]  [bab60cbb-d175-4c45-806f-b05fc08bd5fe] [MCB] async task got an unknown throwable: JSON does not allow non-finite numbers.
[2021-11-11T05:00:49,952][WARN ][o.o.s.l.u.QueryDataAnonymizer] [6a3601a40d1d]  Caught an exception when anonymizing sensitive data
[2021-11-11T05:00:49,952][INFO ][o.o.s.l.p.RestSqlAction  ] [6a3601a40d1d]  [116a891b-0786-4e32-b28b-18219b626782] Incoming request /_plugins/_sql:  select Min ("Apple","Banana") from calcs limit 1;
[2021-11-11T05:00:49,962][WARN ][stderr                   ] [6a3601a40d1d]  line 1:20 no viable alternative at input 'Min ("Apple",'
[2021-11-11T05:00:49,965][WARN ][o.o.s.l.e.AsyncRestExecutor] [6a3601a40d1d]  [116a891b-0786-4e32-b28b-18219b626782] [MCB] async task got an unknown throwable: JSON does not allow non-finite numbers
@Yury-Fridlyand Yury-Fridlyand added Beta bug Something isn't working untriaged labels Nov 11, 2021
@chloe-zh
Copy link
Contributor

chloe-zh commented Nov 11, 2021

@Yury-Fridlyand Thanks for reporting this issue, will take a look and make it fixed once we have resources.

@ghost
Copy link

ghost commented Nov 19, 2021

This issue affects TDVT tests:

  • exprtests/standard\setup.agg.max.txt
  • exprtests/standard\setup.agg.min.txt

@joshuali925 joshuali925 added the tdvt Tableau test framework label Nov 24, 2021
@acarbonetto
Copy link
Collaborator

Reference: https://opensearch.org/docs/latest/opensearch/aggregations/
By default, OpenSearch doesn't perform aggregations on string values. We would have to understand how to map a MAX([strings]) to a valid OpenSearch query (not necessarily an aggregation) before tackling this issue.

Yury-Fridlyand added a commit that referenced this issue Jun 27, 2023
…xt (#1779)

* Fix CI (#1760)

* Fix ML-commons missing dependency.

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>

* Fix `mockito` dependency.

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>

* Revert changes in `:opensearch` since it is not needed anymore.

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>

---------

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
Signed-off-by: Matthew Wells <matthew.wells@improving.com>

* Fix CSV/RAW outputting wrong format (#279)

* Fixed bug where CSV/RAW outputs as JSON rather than plain text

Signed-off-by: Matthew Wells <matthew.wells@improving.com>

---------

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
Signed-off-by: Matthew Wells <matthew.wells@improving.com>
Co-authored-by: Yury-Fridlyand <yury.fridlyand@improving.com>
opensearch-trigger-bot bot pushed a commit that referenced this issue Jun 27, 2023
…xt (#1779)

* Fix CI (#1760)

* Fix ML-commons missing dependency.

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>

* Fix `mockito` dependency.

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>

* Revert changes in `:opensearch` since it is not needed anymore.

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>

---------

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
Signed-off-by: Matthew Wells <matthew.wells@improving.com>

* Fix CSV/RAW outputting wrong format (#279)

* Fixed bug where CSV/RAW outputs as JSON rather than plain text

Signed-off-by: Matthew Wells <matthew.wells@improving.com>

---------

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
Signed-off-by: Matthew Wells <matthew.wells@improving.com>
Co-authored-by: Yury-Fridlyand <yury.fridlyand@improving.com>
(cherry picked from commit 1ec696d)
Yury-Fridlyand pushed a commit that referenced this issue Jun 28, 2023
* Fix CSV/RAW output header being application/json rather than plain/text (#1779)

* Fix CI (#1760)

* Fix ML-commons missing dependency.

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>

* Fix `mockito` dependency.

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>

* Revert changes in `:opensearch` since it is not needed anymore.

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>

---------

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
Signed-off-by: Matthew Wells <matthew.wells@improving.com>

* Fix CSV/RAW outputting wrong format (#279)

* Fixed bug where CSV/RAW outputs as JSON rather than plain text

Signed-off-by: Matthew Wells <matthew.wells@improving.com>

---------

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
Signed-off-by: Matthew Wells <matthew.wells@improving.com>
Co-authored-by: Yury-Fridlyand <yury.fridlyand@improving.com>
(cherry picked from commit 1ec696d)

* updated tests

Signed-off-by: Matthew Wells <matthew.wells@improving.com>

* updated tests to return name

Signed-off-by: Matthew Wells <matthew.wells@improving.com>

* changed tests to return value

Signed-off-by: Matthew Wells <matthew.wells@improving.com>

* removed unneeded imports

Signed-off-by: Matthew Wells <matthew.wells@improving.com>

---------

Signed-off-by: Matthew Wells <matthew.wells@improving.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working powerBI SQL tdvt Tableau test framework
Projects
No open projects
Status: No status
Development

No branches or pull requests

4 participants