Add any existing ext::* scalar to a type's __casttype__
#686
Conversation
| @@ -127,9 +127,15 @@ export function $resolveOverload( | |||
| throw new Error( | |||
| `No function overload found for ${ | |||
| funcName.includes("::") | |||
| ? `'e.${funcName.split("::")[1]}()'` | |||
| ? `'e.${funcName.split("::").join(".")}()'` | |||
There was a problem hiding this comment.
Supports submodules including ext::pgvector in this case. It was printing e.pgvector() instead of e.ext.pgvector.cosine_distance().
Note: we're not dropping std and math here any more 🤔
There was a problem hiding this comment.
Good catch. I guess the previous logic was to strip out the std or default as they get aliased into the top level and we didn't have nested modules, but even that's broken because of the cal and math modules (and any non default user modules) 🤦.
There was a problem hiding this comment.
Yeah, I couldn't quickly think of a maintainable way to update this to still remove std and default other than hardcoding a filter in here, but maybe that's fine?
There was a problem hiding this comment.
Yeah, I think it's fine to always show the full module + function name in error messages. You can probably also shadow the top level aliases of the stdlib functions anyway, and I think it's not worth trying to handle all that correctly just for an error message.
There was a problem hiding this comment.
Actually, I wonder if it's even worth trying to typescript-ify the function names at all 🤔 I think it would be maybe more clear if the error message was pointing to the actual EdgeQL function that was called rather than trying to construct what it probably looks like at the call site? In other words, just use funcName here. Thoughts?
scotttrinh
changed the title
ext::* scalar to a type's __casttype__
| @@ -15,7 +15,7 @@ export async function setupTests() { | |||
| } | |||
|
|
|||
| async function cleanupData(client: Client) { | |||
| await client.execute(`reset schema to initial`); | |||
| await client.execute(`delete PgVectorTest`); | |||
There was a problem hiding this comment.
Whoops! I erroneously thought reset schema to initial just deleted all the user defined objects, but it actually... resets the schema to the initial state before any migrations were run. 🤦
Still need to figure out a query that will delete all objects of user-defined types!
| @@ -127,7 +127,7 @@ export async function getTypes( | |||
| material_scalars := ( | |||
| SELECT ScalarType | |||
| FILTER | |||
| (.name LIKE 'std::%' OR .name LIKE 'cal::%') | |||
| (.name LIKE 'std::%' OR .name LIKE 'cal::%' OR .name LIKE 'ext::%') | |||
There was a problem hiding this comment.
This works for now, but potentially a future extension may add non base-type scalars. So maybe the filter should check that the type has no non-abstract ancestors?
There was a problem hiding this comment.
So maybe the filter should check that the type has no non-abstract ancestors?
Isn't that what AND NOT .is_abstract is doing on line 131? Or do you think there is some way to narrow the search space in ext:: specifically?
There was a problem hiding this comment.
That's checking that the type itself isn't abstract. I meant checking the ancestors are all abstract as well like this:
select schema::ScalarType {
name,
} filter not .abstract
and not exists .enum_values
and not exists (select .ancestors filter not .abstract)
There was a problem hiding this comment.
Oh yeah, maybe I don't understand after all: where would this query go?
There was a problem hiding this comment.
Instead of the current material_scalars query.
There was a problem hiding this comment.
Oh, so don't even filter on the name at all? We don't need the shape here, right? At the very least we also want at least the id since that's what we actually consume. I'll make the change and maybe add a test for std, cal, ext.
There was a problem hiding this comment.
Does this seem right? I left the name filtering in, but do you think that's unnecessary?
There was a problem hiding this comment.
Did a little poking around in my local database and yeah, it seems fine to remove the name filtering here. The only thing it was filtering out was cfg::Memory and that seems fine to include in these "materials".
scotttrinh
force-pushed
the
test-multiple-versions
branch
from
f6836cc
to
da6e8dc
Compare
scotttrinh
force-pushed
the
685-resolve-ext-as-materials
branch
from
66b316d
to
dba99dd
Compare
Checked the results locally, and they seem sensible. Only additional scalar is the `cfg::Memory` scalar, which seems fine to include here
Closes #685
The type system resolves ancestor types to scalars calling them "material scalars" and we were not getting any
ext::*scalars in our subquery that resolves these.Some additional updates to make error messages a bit more useful since they currently print
[object Object]instead of anything useful 😬