Implement Table.replace for the database backend #8986
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
AdRiley
reviewed
Feb 14, 2024
| t . should_equal table | ||
| Problems.expect_warning Empty_Error t | ||
|
|
||
| group_builder.specify "asdfasdfshould throw an error on an empty (but well-formed) lookup table and non-empty base table if allow_unmatched_rows=False" <| |
There was a problem hiding this comment.
Suggested change
| group_builder.specify "asdfasdfshould throw an error on an empty (but well-formed) lookup table and non-empty base table if allow_unmatched_rows=False" <| | |
| group_builder.specify "should throw an error on an empty (but well-formed) lookup table and non-empty base table if allow_unmatched_rows=False" <| |
Typo?
AdRiley
reviewed
Feb 14, 2024
| not, and allow_unmatched_rows=False, throw | ||
| `Unmatched_Rows_In_Lookup` with the first row of the | ||
| table. Otherwise, attach a warning. | ||
| if base_table.row_count == 0 || allow_unmatched_rows then Warning.attach (Empty_Error.Error lookup_table) base_table else |
There was a problem hiding this comment.
Suggested change
| if base_table.row_count == 0 || allow_unmatched_rows then Warning.attach (Empty_Error.Error lookup_table) base_table else | |
| if base_table.row_count == 0 || allow_unmatched_rows then Warning.attach (Empty_Error.Error "lookup_table") base_table else |
Without the quotes the message will contain the contents of the map. Which being empty won't be so informative for the user.
AdRiley
reviewed
Feb 14, 2024
| respectively. | ||
| lookup_table_renamed = selected_lookup_columns . rename_columns (Map.from_vector [[from_column_resolved, duplicate_key_column_name], [to_column_resolved, column]]) | ||
|
|
||
| warn_if_empty result_table = if lookup_table_renamed.row_count != 0 then result_table else Warning.attach (Empty_Error.Error lookup_table_renamed) result_table |
There was a problem hiding this comment.
Suggested change
| warn_if_empty result_table = if lookup_table_renamed.row_count != 0 then result_table else Warning.attach (Empty_Error.Error lookup_table_renamed) result_table | |
| warn_if_empty result_table = if lookup_table_renamed.row_count != 0 then result_table else Warning.attach (Empty_Error.Error "lookup_table") result_table |
AdRiley
reviewed
Feb 14, 2024
| lookup_table = table_builder_typed [['x', []], ['z', []]] Value_Type.Integer | ||
| t = table.replace lookup_table 'x' . order_by ['y'] | ||
| t . should_equal table | ||
| Problems.expect_warning Empty_Error t |
There was a problem hiding this comment.
Suggested change
| Problems.expect_warning Empty_Error t | |
| Problems.expect_warning (Empty_Error "lookup_table") t | |
If you go with my above change. But either way I think our tests should be more specific when testing errors.
AdRiley
previously requested changes
Feb 14, 2024
jdunkerley
approved these changes
Feb 16, 2024
Comment on lines
1573
to
1574
| @from_column Widget_Helpers.make_column_name_selector | ||
| @to_column Widget_Helpers.make_column_name_selector |
There was a problem hiding this comment.
aren't these from the lookup_table so the selector is wrong?
We need to have widgets derived from first argument for this I think.
There was a problem hiding this comment.
I'm not sure what the notation would be for this -- is there documentation for the @ clauses? Or, where is it implemented? I don't see an example of a widget attached to a value other than self.
|
|
||
| ## PRIVATE | ||
| The largest dataset that can be used to make a literal table, expressed in number of elements. | ||
| MAX_LITERAL_ELEMENT_COUNT = 256 |
There was a problem hiding this comment.
256 a reasonable literal default. Agee with @radeusgd should be a temp table when gets bigger!
GregoryTravis
added
the
CI: Ready to merge
GregoryTravis
removed
the
CI: Ready to merge
AdRiley
approved these changes
Feb 16, 2024
GregoryTravis
added
the
CI: Ready to merge
GregoryTravis
added
the
CI: Clean build required
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.
Also implements
Table.replaceusing aMap.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.