Implement Table.replace for the in-memory backend #8935
Conversation
GregoryTravis
requested review from
jdunkerley,
radeusgd and
AdRiley
as code owners
test/Table_Tests/src/Common_Table_Operations/Join/Replace_Spec.enso
Outdated
Show resolved
Hide resolved
test/Table_Tests/src/Common_Table_Operations/Join/Replace_Spec.enso
Outdated
Show resolved
Hide resolved
test/Table_Tests/src/Common_Table_Operations/Join/Replace_Spec.enso
Outdated
Show resolved
Hide resolved
| group_builder.specify "(edge-case) should allow lookup with itself" <| | ||
| table = table_builder [['x', [2, 1, 4, 3]], ['y', [20, 10, 40, 30]]] | ||
| expected = table_builder [['x', [20, 10, 40, 30]], ['y', [20, 10, 40, 30]]] | ||
| result = table.replace table 'x' | ||
| result . should_equal expected |
There was a problem hiding this comment.
I got that from Lookup_Spec.enso 😀
There was a problem hiding this comment.
Looks good, but some comments.
IMO we should not be using Table.new internally too much - there are more 'specialized' operations for these things. Using them will make writing the DB version later on much easier (as in DB we cannot easily Table.new, but we can still set/select_columns/rename_columns/remove_columns easily.
|
|
||
| ## Create a lookup table with just `to_column` and `from_column`, | ||
| renamed to match the base table's `column` and its duplicate, | ||
| respectively. | ||
| lookup_table_renamed = Table.new [lookup_table.at from_column . rename duplicate_key_column_name, lookup_table.at to_column . rename column] | ||
| lookup_table_renamed = lookup_table.select_columns [from_column, to_column] . rename_columns (Map.from_vector [[from_column, duplicate_key_column_name], [to_column, column]]) |
There was a problem hiding this comment.
btw. you are doing this select twice... once here and once above
| @@ -2045,19 +2045,18 @@ type Table | |||
| self.select_columns column . if_not_error <| lookup_table.select_columns [from_column, to_column] . if_not_error <| | |||
There was a problem hiding this comment.
Please do this:
| self.select_columns column . if_not_error <| lookup_table.select_columns [from_column, to_column] . if_not_error <| | |
| selected_lookup_columns = lookup_table.select_columns [from_column, to_column] | |
| self.select_columns column . if_not_error <| selected_lookup_columns . if_not_error <| |
then you can use selected_lookup_columns instead of running the select twice for no need.
There was a problem hiding this comment.
Thanks for addressing most of the issues.
Please consider doing the suggested change to avoid calling select_columns twice unnecessarily (the cost is negligible but it's just not clean to call it twice without need) before merging.
I think ideally it would be good to add the check you suggested - if to_column or from_column has any value other than its default and a Map is provided - that should be an error since the argument should be ignored.
| @column Widget_Helpers.make_column_name_selector | ||
| @from_column Widget_Helpers.make_column_name_selector | ||
| @to_column Widget_Helpers.make_column_name_selector | ||
| replace : Table | Map -> (Text | Integer) -> (Text | Integer | Nothing) -> (Text | Integer | Nothing) -> Boolean -> Problem_Behavior -> Table ! Missing_Input_Columns | Non_Unique_Key | Unmatched_Rows_In_Lookup |
There was a problem hiding this comment.
Let's add support for to Table from a Map.
Then we can take a Table and use conversions.
We can then use 0 for the from_column and 1 for to_column.
There was a problem hiding this comment.
I've enough changes in part 2 (Database Table.replace) that I'd like to change to conversion style after that's done.
| from_column_resolved = from_column.if_nothing 0 | ||
| to_column_resolved = to_column.if_nothing 1 |
There was a problem hiding this comment.
if we use a Map conversion we can avoid needing this.
| ## PRIVATE | ||
| A helper that creates a two-column table from a map. | ||
| map_to_lookup_table : Map Any Any -> Text -> Text -> Table | ||
| map_to_lookup_table map key_column value_column = | ||
| keys_and_values = map.to_vector | ||
| Table.new [[key_column, keys_and_values.map .first], [value_column, keys_and_values.map .second]] | ||
|
|
There was a problem hiding this comment.
we can avoid this if we do the conversion approach.
GregoryTravis
added
the
CI: Ready to merge
GregoryTravis
changed the title
Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
Scala,
Java,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
./run ide build.