-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
sql: compatibility blitz for pgadmin #38869
Conversation
Awesome! This is really great stuff! |
c56566a
to
edaa7b8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @jordanlewis)
pkg/sql/opt/optbuilder/union.go, line 34 at r17 (raw file):
// Try to propagate types left-to-right, if we didn't already have desired // types. if len(desiredTypes) == 0 {
Please add a new test for this in optbuilder/testdata/union
pkg/sql/sem/builtins/builtins.go, line 2246 at r15 (raw file):
), "row_to_json": makeBuiltin(defProps(),
Can you add a logic test for this function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing work, Jordan. This gives further evidence that there's a lot of low-hanging compatibility fruit across various drivers/tools that dedicated engineer(s) could pluck.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @jordanlewis)
5553995
to
f406e6f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Main questionable thing about this is that our type system is a little bit different than Postgres, so when we added the constant upcast path from string to string array, it causes a bad error message to be created for something like
SELECT array['foo'] || bar
. Postgres also errors on this if you don't specify bar::text
. But we produce something like no valid overload from string[] || string
, which is a lie. The reason it produces this is that our typing rules apply in a particular order - we end up filtering out the string[]||string overload early in the case of not knowing the second string's type (it's a constant) because of this rule where we try to type constants homogeneously with resolved types, and throw away the ones that didn't match that. Then, we reject string[]||string[] because the second string can't resolve to a string[] as it doesn't parse to one.
I think the solution would be to change the homogeneous rule to prefer homogeneous but not rule out non-homogeneous, but I think that would be pretty invasive. I'm tempted to leave this bad error message for now.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale)
pkg/sql/opt/optbuilder/union.go, line 34 at r17 (raw file):
Previously, RaduBerinde wrote…
Please add a new test for this in
optbuilder/testdata/union
Done.
pkg/sql/sem/builtins/builtins.go, line 2246 at r15 (raw file):
Previously, RaduBerinde wrote…
Can you add a logic test for this function?
Done.
e5ce5e3
to
1c57c52
Compare
1c57c52
to
199ebfe
Compare
Release note: None
Release note: None
Release note: None
Release note: None
Release note: None
Release note: None
Release note: None
Release note: None
This is supposed to return the last "system OID", which is supposed to represent all OIDs that correspond to objects that were created by the Postgres system, like `pg_class` or all of the default types, so that it's easier to distinguish user-created objects, which will have higher OIDs. Unfortunately for us, we can't properly implement this as our OIDs are made by hash, so system and user OIDs will be all mixed up. To facilitate tools that use this (pgadmin), we always return 0. Release note: None
It was previously erroneously called username. Release note: None
Release note: None
It was previously using the 'public' namespace, but Postgres produces the 'pg_catalog' namespace, indicating that the collations available are from the system. If we had custom collations, those would belong in the 'public' namespace, but we don't. Release note: None
Release note: None
Release note: None
Release note: None
Release note: None
Release note: None
199ebfe
to
0180907
Compare
@RaduBerinde feeling good about your stamp here? I'm ready to merge this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 1 of 0 LGTMs obtained
Thanks a lot for reviewing this! bors r+ |
38869: sql: compatibility blitz for pgadmin r=jordanlewis a=jordanlewis A bunch of small improvements to the catalog, new builtins, and a couple of typing improvements all in the name of making pgadmin work. And it works okay now! Definitely still some flaws and unsupported things, but at least the default experience isn't completely broken anymore. Closes #33341. Closes #23299. Closes #26389. Closes #26378. Closes #26390. Closes #24747. Closes #37124. Updates #25213. ![image](https://user-images.githubusercontent.com/43821/61190353-f17fb980-a668-11e9-947f-d1bc3bb1e75d.png) Co-authored-by: Jordan Lewis <jordanthelewis@gmail.com>
Build succeeded |
A bunch of small improvements to the catalog, new builtins, and a couple of typing improvements all in the name of making pgadmin work.
And it works okay now! Definitely still some flaws and unsupported things, but at least the default experience isn't completely broken anymore.
Closes #33341.
Closes #23299.
Closes #26389.
Closes #26378.
Closes #26390.
Closes #24747.
Closes #37124.
Updates #25213.