Feat refactor shape builder #491
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #491 +/- ##
==========================================
+ Coverage 81.46% 81.49% +0.02%
==========================================
Files 89 89
Lines 19388 19547 +159
Branches 19388 19547 +159
==========================================
+ Hits 15795 15929 +134
- Misses 3175 3200 +25
Partials 418 418 ☔ View full report in Codecov by Sentry. |
Conformance comparison report
Number passing in both: 5587 Number failing in both: 842 Number passing in Base (1b19d11) but now fail: 0 Number failing in Base (1b19d11) but now pass: 0 |
jpschorr
force-pushed
the
feat-refactor-shape-builder
branch
from
0607b05 to
07a5c11
Compare
jpschorr
force-pushed
the
feat-refactor-shape-builder
branch
from
07a5c11 to
2ddd741
Compare
jpschorr
force-pushed
the
feat-refactor-shape-builder
branch
from
2ddd741 to
3e2b6c1
Compare
| @@ -40,7 +40,9 @@ impl BindEvalExpr for EvalStringFn { | |||
| F: Fn(&Box<String>) -> R + 'static, | |||
| R: Into<Value> + 'static, | |||
| { | |||
| UnaryValueExpr::create_typed::<{ STRICT }, _>([type_string!()], args, move |value| { | |||
| // use DummyShapeBuilder, as we don't care about shape Ids for evaluation dispatch | |||
| let mut bld = DummyShapeBuilder::default(); | |||
There was a problem hiding this comment.
Since the usage is going beyond just tests perhaps it's more appropriate to rename the DummyShapeBuilder.
There was a problem hiding this comment.
Probably a good idea. Any suggestions?
There was a problem hiding this comment.
Not sure how good these are, just putting them out there :)
PartiqlNoIdShapeBuilder, NoIdShapeBuilder, StaticIdShapeBuilder
| @@ -5,7 +5,7 @@ use crate::eval::eval_expr_wrapper::{ | |||
| use crate::eval::expr::{BindError, BindEvalExpr, EvalExpr}; | |||
| use crate::eval::EvalContext; | |||
|
|
|||
| use partiql_types::{PartiqlShapeBuilder, StructType}; | |||
| use partiql_types::{DummyShapeBuilder, PartiqlShapeBuilder, StructType}; | |||
There was a problem hiding this comment.
| use partiql_types::{DummyShapeBuilder, PartiqlShapeBuilder, StructType}; | |
| use partiql_types::DummyShapeBuilder; |
Based on clippy, it looks like PartiqlShapeBuilder and StructType are unused.
jpschorr
force-pushed
the
feat-refactor-shape-builder
branch
from
a915a13 to
eb7233c
Compare
I had issues practically using Shapes with Ids in an external project. I began fixing one thing, and ended up with this refactor.
Esentially, this removes usages of
Arcs and allows control of theIDgeneration by using a builder that is threaded through type creation. While this is slightly less ergonomic, it allows serialization/deserialization, test cases, etc.In addition to these changes, I also think that we will need to add
IDs to, for example, undefined and dynamic types, not just static in the future.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.