Redefine FieldName as (TableId, ColId)
#1165
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Centril
force-pushed
the
centril/simplify-fieldname
branch
from
7d231a5 to
d96e452
Compare
Centril
commented
Apr 26, 2024
| @@ -351,7 +349,7 @@ impl InstanceEnv { | |||
| let tx = &mut *self.tx.get()?; | |||
|
|
|||
| let schema = stdb.schema_for_table_mut(tx, table_id)?; | |||
| let row_type = ProductType::from(&*schema); | |||
| let row_type = schema.get_row_type(); | |||
There was a problem hiding this comment.
drive-by optimization
Comment on lines
+201
to
+207
| let mut iter = ident.rsplit('.'); | ||
| let field = iter.next(); | ||
| let table = iter.next(); | ||
| let more = iter.next(); |
There was a problem hiding this comment.
Avoids allocation now.
Comment on lines
+223
to
+230
| if let Some(f_table) = f_table { | ||
| // Qualified field `{f_table}.{f_field}`. | ||
| // Narrow search to first table with name `f_table`. | ||
| return if let Some(col) = tables | ||
| .find(|t| &*t.table_name == f_table) |
There was a problem hiding this comment.
Avoids allocation in the qualified path now.
| @@ -626,10 +630,9 @@ fn compile_select_item(from: &From, select_item: SelectItem) -> Result<Column, P | |||
| fn compile_select<T: TableSchemaView>(db: &RelationalDB, tx: &T, select: Select) -> Result<SqlAst, PlanError> { | |||
| let from = compile_from(db, tx, &select.from)?; | |||
| // SELECT ... | |||
| let mut project = Vec::new(); | |||
| let mut project = Vec::with_capacity(select.projection.len()); | |||
There was a problem hiding this comment.
Drive-by optimization
Comment on lines
-53
to
-78
| fn check_field(table: &From, field: &FieldExpr) -> Result<(), PlanError> { | ||
| if let FieldExpr::Name(field) = field { | ||
| table.find_field(&field.to_string())?; | ||
| } | ||
| Ok(()) | ||
| } | ||
|
|
||
| fn check_field_column(table: &From, field: &ColumnOp) -> Result<(), PlanError> { | ||
| if let ColumnOp::Field(field) = field { | ||
| check_field(table, field)?; | ||
| } | ||
| Ok(()) | ||
| } | ||
|
|
||
| /// Verify the `fields` inside the `expr` are valid | ||
| fn check_cmp_expr(table: &From, expr: &ColumnOp) -> Result<(), PlanError> { | ||
| match expr { | ||
| ColumnOp::Field(field) => check_field(table, field)?, | ||
| ColumnOp::Cmp { op: _, lhs, rhs } => { | ||
| check_field_column(table, lhs)?; | ||
| check_field_column(table, rhs)?; | ||
| } | ||
| } | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
By having FieldName we've already verified, so no need to do it again.
| @@ -529,7 +532,8 @@ fn compile_from<T: TableSchemaView>(db: &RelationalDB, tx: &T, from: &[TableWith | |||
|
|
|||
| match constraint { | |||
| JoinConstraint::On(x) => { | |||
| let expr = compile_expr_value(&base, None, x.clone())?; | |||
| let tables = base.iter_tables().chain([&*join]); | |||
There was a problem hiding this comment.
This is the reason find_field had to become a free function.
We must extend the context with the joined table before adding it to the returned base.
Centril
force-pushed
the
centril/simplify-fieldname
branch
4 times, most recently
from
af99b17 to
5b5274a
Compare
Centril
commented
Apr 26, 2024
Comment on lines
615
to
620
| let result = run_for_testing( | ||
| &db, | ||
| "insert into inventory (id, name) values (1, 'Kiley'); | ||
| insert into inventory (id, name) values (2, 'Terza'); | ||
| insert into inventory (id, name) values (3, 'Alvie'); | ||
| "insert into inventory (inventory_id, name) values (1, 'Kiley'); | ||
| insert into inventory (inventory_id, name) values (2, 'Terza'); | ||
| insert into inventory (inventory_id, name) values (3, 'Alvie'); | ||
| SELECT * FROM inventory", |
There was a problem hiding this comment.
Seems like we have a bug in master as the column name is inventory_id and not id.
Centril
commented
Apr 26, 2024
| .iter() | ||
| .map(|x| { | ||
| let ty = x.algebraic_type.clone(); | ||
| let name = translate_col(tx, x.field); |
There was a problem hiding this comment.
We have to do this to match the old output as the header no longer contains the column names.
Centril
force-pushed
the
centril/simplify-fieldname
branch
from
5b5274a to
dc59598
Compare
Centril
force-pushed
the
centril/simplify-fieldname
branch
from
dc59598 to
1ce322c
Compare
bfops
added
breaks library compatibility
Centril
force-pushed
the
centril/simplify-fieldname
branch
from
578bd6c to
4b99880
Compare
gefjon
approved these changes
Apr 30, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of Changes
Do not store table names and field names in
FieldName.Rather, store the table id and the column id which is substantially cheaper (8 bytes).
Fixes a bug in
fn column_posalong the way and does some drive-by cleanup.The second commit adds both necessary changes to tests and refactoring around them, and similar changes which I had to do to understand and be confident in the changes to the tests. The diff got larger than I wanted, but once merged, I think it will help us long term.
Fixes #1118.
Fixes #1117.
API and ABI breaking changes
None
Expected complexity level and risk
2