Skip to content

bugfix: add neq to comparison operators map #540

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

Merged
merged 5 commits into from
Jul 23, 2024
Merged

bugfix: add neq to comparison operators map #540

merged 5 commits into from
Jul 23, 2024

Conversation

soupi
Copy link
Contributor

@soupi soupi commented Jul 19, 2024

What

Some databases do not include != in their catalog's operators list. We include <> as well so that _neq can be generated for these cases.

How

  1. Add it to the list

@soupi soupi changed the title add neq to comparison operators map bugfix: add neq to comparison operators map Jul 19, 2024
@soupi soupi requested a review from plcplc July 22, 2024 11:11
@@ -1495,7 +1495,7 @@ WITH
WITH
comparison_infix_operators_mapped AS
(
SELECT
SELECT DISTINCT ON (map.exposed_name, op.*)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'd need to update the introspection query of v3 and v4 too if you make changes to them.

IIRC The code has to solve a similar disambiguation problem in the section that deals with implicit cast extension. There it uses LIMIT + ROW_NUMBER to emulate DISTINCT ON + ORDER BY, because cockroachdb just throws ordering of nested queries out the window (see the commentary there)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I think we should leave v3 and v4 completely untouched. We shouldn't service them at all unless for a security issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the distinct on as we agreed that it's not necessary because each database only defines one variant of the neq operator.

}
]
}
}
},
"NativeQueries": {
"description": "Metadata information of Native Operations.",
"description": "Metadata information of Native Queries.",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a change we want to make? (IIRC the sanctioned name is "Native Operations").

It looks like this file hasn't been updated for some time and has collected a delta.

Copy link
Contributor Author

@soupi soupi Jul 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this file hasn't been updated for some time and has collected a delta.

You are right.

Is this a change we want to make? (IIRC the sanctioned name is "Native Operations").

Yes - in config v5, we split native queries to two separate fields and nest them under native operations. This particular bit represents the queries field, so it makes sense to call it native queries. The mutations field is called native mutations as well, and they both nest under the umbrella term Native Operations:

"NativeOperations": {
"description": "Metadata information of Native Operations.",
"type": "object",
"required": ["mutations", "queries"],
"properties": {
"queries": {
"description": "Native Queries.",
"allOf": [
{
"$ref": "#/definitions/NativeQueries"
}
]
},
"mutations": {
"description": "Native Mutations.",
"allOf": [
{
"$ref": "#/definitions/NativeMutations"
}
]
}
}
},

@@ -46,6 +46,11 @@ impl ComparisonOperatorMapping {
exposed_name: "_lt".to_string(),
operator_kind: OperatorKind::Custom,
},
ComparisonOperatorMapping {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should avoid making updates to old versions of the configuration format.

In this particular case it probably doesn't matter much either way, since the operator name mapping is just an introspection option, and all we are changing is the default value. Since we stamp out the defaults when we initialize a new configuration only people who explicitly refresh their defaults or create a new configuration with an old version number will ever see these changes.

But generally we should avoid it and prefer users upgrade ther configuration instead.

In principle this change could have to incur an update of the documentation of the configuration reference section.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with you in principle, and changing this won't make a big difference because initialize will always output v5 configs, but it would make me feel better if we do this in this particular case :)

@soupi soupi enabled auto-merge July 23, 2024 09:28
@soupi soupi added this pull request to the merge queue Jul 23, 2024
Merged via the queue into main with commit 095ecbf Jul 23, 2024
30 checks passed
@soupi soupi deleted the gil/add-neq branch July 23, 2024 09:39
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