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

support "NULLS LAST" in "ORDER BY" clauses #468

Open
BillFarber opened this issue Feb 23, 2023 · 4 comments
Open

support "NULLS LAST" in "ORDER BY" clauses #468

BillFarber opened this issue Feb 23, 2023 · 4 comments

Comments

@BillFarber
Copy link

It would be helpful if Winnow supported the "NULLS LAST" clause that is available with SQL "ORDER BY" clauses.

When our provider, koop-provider-marklogic (https://github.com/koopjs/koop-provider-marklogic), retrieves paged data and there is an "ORDER BY" field that contains NULL values, then there is likely to be a page of data with a mix of values and nulls. The orderBy clause generated by "createOrderByClause" in "order-by.js" does not recognize the "NULLS LAST" clause and therefore on a page with mixed values, the NULLS appear first in the list, even though they appear last in query response from the database.

We would like to be able to include "NULLS LAST" in the "ORDER BY" clause and then have the SQL generated in Winnow respect that.

I'm not intending to propose the right solution here, but I did find out that the below change supports what we're looking for

function createOrderByClause (options = {}) {
  const { order: orderByArray, esri, aggregates } = options;
  if (!orderByArray) return '';

  const selector = esri ? 'attributes' : 'properties';

  const orderByClause = orderByArray.map(orderBy => {
    let [field, direction = 'ASC'] = orderBy.split(' ');
    // New code
    if (orderBy.toUpperCase().endsWith("NULLS LAST")) {
      direction += " NULLS LAST";
    }
    // End of new code
    if (shouldFormatForAggregationQuery(field, aggregates)) {
      return `\`${field}\` ${direction.toUpperCase()}`;
    }
    return `${selector}->\`${field}\` ${direction.toUpperCase()}`;
  }).join(', ');

  return ` ORDER BY ${orderByClause}`;
}
@rgwozdz
Copy link
Member

rgwozdz commented Feb 24, 2023

Hello @BillFarber - thanks for reaching out with this issue. I think something like this could work. But I will note that the Geoservices specification (i.e., ArcGIS Feature Service Query) doesn't support NULLS LAST (as far as I can tell anyway). Are your Koop requests being issue by a client that adds NULLS LAST? Or are you using this package (Winnow) in a novel way?

@jkerr5
Copy link

jkerr5 commented Mar 3, 2023

Is there documented behavior for Esri that says whether it orders nulls first or last? It looks like the SQL default is nulls first so is that what Esri assumes?

Maybe the ask is for a "sort" option for "filtersApplied" so winnow doesn't reorder the results from the provider?

@rgwozdz
Copy link
Member

rgwozdz commented Mar 3, 2023

Maybe the ask is for a "sort" option for "filtersApplied" so winnow doesn't reorder the results from the provider?

good idea, I think that's the way to go, assuming the order of nulls currently matches ArcGIS. I will see if I can ascertain that ordering.

@rgwozdz
Copy link
Member

rgwozdz commented Mar 24, 2023

Looks like default is for Nulls to go last:

https://maps2.dcgis.dc.gov/dcgis/rest/services/DCGIS_DATA/Environment_WebMercator/MapServer/23/query?where=1%3D1&outFields=CONDITION&orderByFields=CONDITION

The CONDITION field has null values, but they don't appear in the first 1000 results at least.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants