search

elastic / elasticsearch

Free and Open Source, Distributed, RESTful Search Engine
https://www.elastic.co/products/elasticsearch
Other
1.07k stars 24.84k forks source link

ES|QL: functions using "asc"/"desc" as arguments should switch to parser-based validation #112386

Open astefan opened 2 months ago

astefan commented 2 months ago

Description

mv_sort and top use "asc"/"desc" as constants for one of their parameters. These are very fragile and could easily lead to hard-to-debug issues.

Case in question: top should implement an improved variant of the surrogate() method that should deal with foldable values (like other aggregate functions). Likely, the formula for top should be mv_slice(mv_sort(field, "asc"/"desc"), start, end) BUT in case the manually created Literal as "asc"/"desc" doesn't have the proper data type (an easy to implement bug while coding), an issue I stumbled upon led me to PruneColumns.removeUnused method that, when doing if (used.contains(prev.toAttribute()) == false) the resolveType() method of Top is called which in turn will "validate" "asc"/"desc" values as incorrect because of the wrong data type (the bug I mentioned above) is checked and PruneColumns will remove the incorrect column because of this.

Instead, "asc"/"desc" should be parsed appropriately in the parser and use one of the builtin constructs - the "asc"/"desc" sort already uses, through OrderBy, Order and OrderDirection classes.

elasticsearchmachine commented 2 months ago

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

elasticsearchmachine commented 2 months ago

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