[BUG] Unimplemented PPL Sort Syntax

[currantw]

What is the bug?

PPL supports syntax for sorting numerically, lexicographically, or by IP address (e.g. sort num(field_name), sort str(field_name), and sort ip(field_name), respectively) -- but this syntax has no effect on the resulting sort order. It should be removed.

This syntax appears to have been replaced by the addition of data types, and sorting is always done according to the data type (i.e. numerical data types are sorted numerically, strings are sorted lexicographically), rather than as specified by str, num, or ip (i.e. these keywords have no effect on the result). Moreover, this syntax is not mentioned in the supporting user documentation for either OpenSearch SQL or Spark, and there is no actual implementation of the functionality in either code bases - the specified "sort type" is simply ignored.

How can one reproduce the bug?

Steps to reproduce the behaviour:

1. Create data set with numerical field
2. Sort using the str keyword (i.e. sort str(field_name))
3. Observed: syntax is valid, but sorting is still numerical

What is the expected behaviour?

As a user, you would likely expect the result to be sorted as specified (i.e. numerically, lexicographically, or by IP address), but the actual behaviour is to always sort by the field's data type.

If a user still wants to sort a numerical field lexicographically (for example) once this syntax is removed, they can still do so by first casting the field to a numerical data type before sorting it.

What is your host/environment?

N/A

Do you have any screenshots?

None

Do you have any additional context?

Related to #3145 (Add IP data type) and opensearch-project/opensearch-spark#963 (same issue for Spark).

[Swiddis]

Thanks for the bug report!

Where exactly do we explain this syntax in the docs? I see from the Spark PR that there's some documentation examples without explanations, I guess those are vestigial examples that imply it's a leftover grammar quirk.

If it's a grammar quirk, I'm mostly concerned with the possibility of a breaking change when people previously used these queries fine (and perhaps expected them to work) after copying the examples. I think the "safe" route is to mark these functions as deprecated no-ops in the documentation/as part of the response for such queries, clean out other doc references to them, and then only remove them from the grammar next major release.

[Swiddis]

In line with the above, I think this is more a documentation issue than a bug, since there's a clear workaround for the faulty behavior and the old sort operations haven't been supported for a very long time (or perhaps ever?). I'm not sure there's significant harm in letting the no-ops exist in peace.

[anasalkouz]

I agree with @Swiddis. I think we can remove it from our documentation safely. Removing them from the grammar might cause a breaking change for customers, we can remove those changes as part of our next major release.

[currantw]

For clarity, here is a test case that illustrates the bug:

Data

Dataset: addresses

street_number (string)	street_name (string)
725	Main Street
1127	Arbutus Street
43	Victoria Drive

Query

search source=addresses | sort num(street_number) | fields street_number, street_name

Expected Output

street_number	street_name
43	Victoria Drive
725	Main Street
1127	Arbutus Street

Actual Output

street_number	street_name
1127	Arbutus Street
43	Victoria Drive
725	Main Street

[currantw]

I agree with @Swiddis. I think we can remove it from our documentation safely. Removing them from the grammar might cause a breaking change for customers, we can remove those changes as part of our next major release.

Thanks @anasalkouz and @Swiddis. Agreed, makes more sense to avoid introducing a breaking change.

The existing syntax seems to be modelled on Spunk (Splunk sort documentation). If it is desirable to keep this syntax, for ease of use for those familiar with Splunk, we could alternately fix it so that it does work. This would be relatively easily to implement, since these sort field keywords could simply work as a "shorthand" for casting (see below).

With Sort Field Keyword	With Cast Function
sort auto(field_name)	sort field_name
sort num(field_name)	sort cast(field_name as double)
sort str(field_name)	sort cast(field_name as string)
sort ip(field_name)	sort cast(field_name as ip)

Please let me know what you think!

[currantw]

Thanks for the bug report!

Where exactly do we explain this syntax in the docs? I see from the Spark PR that there's some documentation examples without explanations, I guess those are vestigial examples that imply it's a leftover grammar quirk.

If it's a grammar quirk, I'm mostly concerned with the possibility of a breaking change when people previously used these queries fine (and perhaps expected them to work) after copying the examples. I think the "safe" route is to mark these functions as deprecated no-ops in the documentation/as part of the response for such queries, clean out other doc references to them, and then only remove them from the grammar next major release.

@Swiddis I don't that this syntax is actually referenced anywhere in the current documentation for the SQL project, which makes it less likely that customers may depend on it/have unwittingly copied it. But I don't know if it may have been part of the documentation in the past, so it's probably best to, as you suggest, wait until the next major release to remove it (if we actually do want to remove it - see my comment above).

[Swiddis]

I like the idea of implementing it as a shorthand, type(var) == cast(var as type) is exactly how I had read the syntax in isolation anyways (as opposed to any extra semantics that may exist with "sort type"). I'll mark the issue as an enhancement, not sure what kind of timeline we're looking at to prioritize it.

I'm willing to mark this as help-wanted from external contributors since it shouldn't be very complex, if we add some links to the relevant code pieces we can also consider marking this as a good first issue.

[currantw]

I like the idea of implementing it as a shorthand, type(var) == cast(var as type) is exactly how I had read the syntax in isolation anyways (as opposed to any extra semantics that may exist with "sort type"). I'll mark the issue as an enhancement, not sure what kind of timeline we're looking at to prioritize it.

I'm willing to mark this as help-wanted from external contributors since it shouldn't be very complex, if we add some links to the relevant code pieces we can also consider marking this as a good first issue.

@Swiddis Makes sense to me.

Some additional thoughts/considerations:

• Would it make sense to generalize this to all data types (i.e. add a "shorthand" method for each supported data type)? This could obviously be completed as part of a separate follow-up issue.
• This will require updating the PPL syntax. Currently, the syntax is quite restrictive: it only allows sorting by qualified field name - for example, the expression sort cast(field as integer) actually isn't valid. Rather than updating this PPL syntax to only support the "shorthand" sort syntax discussed, would it be better to allow sorting on any expression?
• Should auto be removed completely? It doesn't do anything (it doesn't alter the behaviour, since sorting by the field's data type is the default), and potentially is more confusing if all the other shorthands correspond to data types - or does the parallel with Splunk make it worth keeping anyway?
• What about num? We don't have a general "number" data type, only more specific numerical types (byte, double, float, long, short, and integer). Should we add shorthand for each of these types (e.g. byte(), double(), float(), long(), short(), and int())? Should we remove num (since it might lead to unexpected results), or should we keep it (for consistency with Splunk) and just cast to double "under the hood"?

Based on your answers to the above, perhaps it would make sense to tackle this in the following steps:

1. Update syntax to support sorting by any expression (e.g. sort cast(field as integer)).
2. Add "shorthand" data casting methods str, ip, byte, double, float, long, short, and int (e.g. sort int(field)). Remove support for auto and num sort syntax.
3. Add "shorthand" methods for remaining data types: bool, date, time, timestamp. (And perhaps json as well. See [FEATURE] Add json function to cast string to json object #3209). (e.g. sort timestamp(field)).

Let me know what you think!

[Swiddis]

I think num is probably fine since in principle sorting by all of the numeric types listed should be the same logic: everything listed would have equivalent semantics, except perhaps the Weird floating-point values. Maybe steal the OrderedFloat semantics? We can consider breaking it up for consistency but I don't see it as an issue.

I'm in favor of the other language changes you proposed (extending the PPL sort syntax, more sort types, and removing auto) and the plan you provided. If the system for sorting is sufficiently general, this may all be doable in one go, I'm not sure how that system looks though. Might be worth getting input from someone who knows this part of the system better.

One thing I have run into is that using complex sort logic in OpenSearch queries can kill performance, from experience even something as simple as going from sorting by one field -> sorting by two fields (primary and secondary) will slow down queries by ~6x. For large enough indices this will cause query failures, so an implementation of arbitrary sorting expressions should be robust against resource exhaustion.