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

Minor QOL improvements #46

Closed
wants to merge 3 commits into from
Closed

Minor QOL improvements #46

wants to merge 3 commits into from

Conversation

jtfell
Copy link
Contributor

@jtfell jtfell commented Nov 15, 2020

Just a few minor tweaks that have been helpful in my project (it's quite spatial and text search heavy). I'm happy to split it up into multiple PRs if you would prefer but I think its mostly uncontroversial.

  • Added some distance operator (<->) related conditions (plus an ordering one that doesn't quite fit as a "condition" but maybe its okay to sneak it in alongside the or and and ones).
  • Added a performance monitoring parameter to the resultListener callback. This has been particularly helpful debugging query performance and will likely be useful for general logging down the track
  • I hit an issue where I was constructing a where clause like this:
const res = await db.select('cats', { breed: params?.breed }).run(pgPool);

but this didn't work properly as the undefined string ended up in the generated SQL. I added in a check to just ignore the condition if its undefined.

@jawj
Copy link
Owner

jawj commented Nov 16, 2020

Thanks, I'll have a look at these.

I assume your distance checks are on a text-similarity metric, otherwise you'd be using ST_DWithin?

@jtfell
Copy link
Contributor Author

jtfell commented Nov 16, 2020

It works for text-similarity (using pg_trgm) and for geography distance. Eg. to sort based on how far the results are from a specified point:

const makePoint = (lat: number, lon: number): db.SQLFragment =>
  db.sql`ST_SetSRID(ST_MakePoint(${db.param(lon)}, ${db.param(lat)}), 4326)::geography`;

const order = {
  by: dc.distanceOrder(db.sql`${'geom'}::geography`, makePoint(50, 50)),
  direction: 'ASC'
};

// The 10 closest cats to your location
const res = await db.select('cats', db.all, {
  order,
  limit: 10
});

This will work using an index USING GIST (geography(geom));

@jtfell jtfell changed the base branch from no-copy to master November 17, 2020 07:08
@jawj
Copy link
Owner

jawj commented Nov 17, 2020

  • Added some distance operator (<->) related conditions (plus an ordering one that doesn't quite fit as a "condition" but maybe its okay to sneak it in alongside the or and and ones).

I don't want the codebase to become too sprawling, and I'm not 100% sold on the these, so perhaps for now you could keep them in your own condition helpers module?

  • Added a performance monitoring parameter to the resultListener callback. This has been particularly helpful debugging query performance and will likely be useful for general logging down the track

This is a great idea: will definitely implement.

  • I hit an issue where I was constructing a where clause like this:
const res = await db.select('cats', { breed: params?.breed }).run(pgPool);

but this didn't work properly as the undefined string ended up in the generated SQL. I added in a check to just ignore the condition if its undefined.

My preferred behaviour here would be for it to be a type error to explicitly pass undefined as a Whereable value — because it feels like the intention could be ambiguous — but it seems like this just isn't possible.

Failing that, I'm not sure whether to convert undefined values into an IS NULL check, throw an error, or do what you did and just delete the condition. I'm not entirely happy with any of those options.

In the meantime, I guess you'll continue need something like:

const 
  breed = params?.breed,
  res = await db.select('cats', breed ?  { breed } : db.all).run(pgPool);

or:

const 
  breed = params?.breed,
  res = await db.select('cats', { ...(breed ?  { breed } : {}), otherCharacteristic: true }).run(pgPool);

@jawj
Copy link
Owner

jawj commented Nov 17, 2020

@jtfell If you can give me a PR with just the timing additions to core.ts I'll be happy to merge — otherwise no big deal for me to just extract those changes manually.

@jtfell
Copy link
Contributor Author

jtfell commented Nov 18, 2020

I don't want the codebase to become too sprawling, and I'm not 100% sold on the these, so perhaps for now you could keep them in your own condition helpers module?

Ok no worries, I'll remove them.

Failing that, I'm not sure whether to convert undefined values into an IS NULL check, throw an error, or do what you did and just delete the condition. I'm not entirely happy with any of those options.

It looks like Sequelize throws an error in this scenario and converts null values into IS NULL. This seems like reasonable behaviour. I've updated the PR to mirror this for now, let me know and I'll just get rid of it.

I'm not in a particular rush to get any of this merged so happy to discuss further rather than just merging the timing part if thats ok with you?

@jawj
Copy link
Owner

jawj commented Nov 23, 2020

I've pondered this a bit. I don't think there's anything wrong with the throwing-on-undefined approach, but I'm also not definite whether it's the best approach, so I feel like erring on the side of not changing the pg library's semantics for now (pg treats undefined as null).

@jawj jawj closed this in e8bd5c2 Nov 25, 2020
@jtfell
Copy link
Contributor Author

jtfell commented Nov 25, 2020

I feel like erring on the side of not changing the pg library's semantics for now

This sounds reasonable to me. Do you mind if I create a new PR with only the timing change?

@jawj
Copy link
Owner

jawj commented Nov 26, 2020

@jtfell, in a fit of tidying up I ended up making this change myself in order to close the ticket.

@jawj
Copy link
Owner

jawj commented Nov 26, 2020

And it's now in 3.1.1.

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

Successfully merging this pull request may close these issues.

2 participants