select: return error if field doesn't exist on tables with strict schema #477
Conversation
add path analyzer.
Codecov Report
@@ Coverage Diff @@
## main #477 +/- ##
==========================================
+ Coverage 79.09% 79.12% +0.03%
==========================================
Files 128 128
Lines 10748 10822 +74
==========================================
+ Hits 8501 8563 +62
- Misses 1543 1549 +6
- Partials 704 710 +6
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
asdine
changed the title
There was a problem hiding this comment.
Thanks @tzzed !
The problem with this approach is that it's a hack: we are generating a string representation of an expression just because it's easier to analyze it while parsing it.
The thing is it has already been parsed before this function was called, and now we are parsing it again.
// Lifecycle of a query
Query string -> parser -> prepare -> exec stream
We should instead simply walk over all expressions and use GetFieldConstraintForPath to see if it's declared or not.
for _, pe := range stmt.ProjectionExprs {
expr.Walk(pe, func(e expr.Expr) bool {
switch t := e.(type) {
case expr.Path:
if fc := ti.FieldConstraints.GetFieldConstraintForPath(document.Path(t)); fc == nil {
// return an error
}
}
})
}Perhaps make this a function and use it for all expressions of the SELECT statement.
| {"EXPLAIN SELECT a + 1 FROM test WHERE c > 10 AND d > 20", false, `"table.Scan(\"test\") | docs.Filter(c > 10) | docs.Filter(d > 20) | docs.Project(a + 1)"`}, | ||
| {"EXPLAIN SELECT a + 1 FROM test WHERE c > 10 OR d > 20", false, `"table.Scan(\"test\") | docs.Filter(c > 10 OR d > 20) | docs.Project(a + 1)"`}, | ||
| {"EXPLAIN SELECT a + 1 FROM test WHERE c IN [1 + 1, 2 + 2]", false, `"table.Scan(\"test\") | docs.Filter(c IN [2, 4]) | docs.Project(a + 1)"`}, | ||
| {"EXPLAIN SELECT a + 1 FROM test WHERE c > 10", true, ``}, |
There was a problem hiding this comment.
These tests were written to test something specific, we should not disable them by expecting them to fail.
We should instead either:
- change the declaration of the table so that it's not a strict table anymore
- change the path
cwith something that is already declared - declare missing paths in the table declaration
asdine
force-pushed
the
main
branch
2 times, most recently
from
3e5896b to
69da8a8
Compare
|
Closing this as the codebase have changed quite a bit, and this is no longer neeed. Thanks @tzzed ! |
This PR fixes #465.