ABI v9 validation code #1572
ABI v9 validation code #1572
Conversation
kazimuth
force-pushed
the
jgilles/validation
branch
2 times, most recently
from
bb4d9c4 to
c10127a
Compare
|
@Centril @cloutiertyler This is ready for review. |
|
I didn't touch C#, so not sure where those SDK failures are coming from. |
kazimuth
force-pushed
the
jgilles/validation
branch
2 times, most recently
from
485f7da to
893e373
Compare
kazimuth
force-pushed
the
jgilles/validation
branch
from
2dba1cf to
058f3d7
Compare
Apply suggestions from code review Co-authored-by: Mazdak Farrokhzad <[email protected]> Signed-off-by: james gilles <[email protected]> Address review comments Address more review comments Remove dead code Fix tests Add primary key support Fix #1591 by handling scoped type names explicitly Update validate for new ColList API Whoops, that derive was wrong Better ColList usage Updates that Phoebe pointed out Whoops, removed something we needed Test that indexes are generated correctly in v9 Whoops, forgot to update raw_def
kazimuth
force-pushed
the
jgilles/validation
branch
from
e1afd15 to
f695d0d
Compare
|
@Centril @cloutiertyler I consider this ready for merge. |
Signed-off-by: james gilles <[email protected]>
| // Alias provided? Relate `name -> slot_ref`. | ||
| if let Some(name) = name { | ||
| // Right now, we just split the name on patterns likely to split up module paths. | ||
| // TODO(1.0): build namespacing directly into the bindings macros so that we don't need to do this. |
There was a problem hiding this comment.
Note: this will happen when we are redoing the Rust binding macros to return v9. It will not be an ABI break because the TypespaceBuilder traitTypespaceBuilder lives inside the module, so it is not part of the ABI. It is part of the API surface touched by the macros, so this will be an API break, but it's one of those weird actually-private-despite-being-formally-public APIs you get with macros.
| types: HashMap<ScopedTypeName, TypeDef>, | ||
|
|
||
| /// The typespace of the module definition. | ||
| typespace: Typespace, |
There was a problem hiding this comment.
When we add AlgebraicTypeUse and AlgebraicTypeDefinition, we may want to change this to a datastructure that stores those for client codegen. On the other hand, I think the database engine still wants to think about algebraic types structurally, so we may be in the uncomfortable position of needing both.
There was a problem hiding this comment.
I think in the future we will want to think about how we can implement nominal types within a structural type system on a holistic level.
There was a problem hiding this comment.
Yeah, I think that would be very worthwhile.
| pub mod validate; | ||
|
|
||
| /// A map from `Identifier`s to values of type `T`. | ||
| pub type IdentifierMap<T> = HashMap<Identifier, T>; |
There was a problem hiding this comment.
As far as I can tell, all the types you store here implement your nice ModuleDefLookup trait and already allow to get a map key from T.
One thing I've done in the past for similar scenarios is use HashSet instead + a newtype that implements Eq + Hash + Borrow<Key> by delegating to the key comparisons.
That way you should be able to avoid a lot of allocations in all these (foobar.name.clone(), foobar) KV construction lines, as well as save some memory by simply reusing foobar itself as both the key and the value of each entry.
Idk how critical it is, so feel free to leave this as a future optimisation, just thought I'd throw it out there as I noticed a lot of those clones in the PR.
There was a problem hiding this comment.
Oh that's a great idea. That should get rid of almost all of these clones and get rid of some possible invalid states.
I'm tempted to leave it as a future optimization for now though just in favor of getting this merged.
Description of Changes
WIP validation code for moduledefv9, plus the definition for
ModuleDef. The new module-def should also be backwards-compatible withV8BackCompatand the existingTableSchema,Header, etc types. I have not implemented those bridges yet.API and ABI breaking changes
A few API changes in components I have recently added, otherwise this is just adding code.
Expected complexity level and risk
1, there is a good amount of code but I am generally happy with the redesigned interfaces. It's fun playing with my new multi-error combinator.
Testing
Test porting is WIP, I have more proptest helpers now and I am going to use those to fuzz this once I've got the rest of the stuff ported.