Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expand capabilities of Table.set and better dropdown support, #8005

Merged
merged 35 commits into from Oct 13, 2023

Conversation

jdunkerley
Copy link
Member

@jdunkerley jdunkerley commented Oct 9, 2023

Pull Request Description

  • Adds the ability to use numbers, date/time and Boolean values as constants in set.
  • Table.set can take a Column_Operation, allowing for deriving of a new column based on other columns.
  • Added Column_Ref type to refer to a column in filter.

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed, the GUI was tested when built using ./run ide build.

@jdunkerley jdunkerley changed the title Add derive_value and if to the Table. Expand capabilities of Table.set and better dropdown support, Oct 11, 2023
@radeusgd
Copy link
Member

radeusgd commented Oct 11, 2023

ToDo

  • Add Is_In and Not+In
  • Add Mod and Power Column_Operation wrapping % and /.
  • Add Trim to Column_Operation.
  • Add all the missing tests 🙂
  • If needs default constants for true and false args
  • Switch input to be a Column_Ref or Any - follow the same logic as in set but no support for an expression or a Column_Operation.
  • Add Min and Max as binary mode operations.
  • Verify widgets work:
    • Is_In / Not_In
    • Min / Max
    • Mod / Power
    • Trim
    • format

Follow up tickets

image

@enso-bot
Copy link

enso-bot bot commented Oct 13, 2023

Radosław Waśko reports a new STANDUP for yesterday (2023-10-12):

Progress: Added missing parts and tests to derive columns. It should be finished by 2023-10-13.

Next Day: Next day I will be working on the same task. Do some cleanup, verify widgets work. Create followup tasks.

@radeusgd radeusgd marked this pull request as ready for review October 13, 2023 10:39
Comment on lines +60 to +61
Filter_Condition.Is_In values -> Filter_Condition.Is_In (check_is_in_values "Is_In" values)
Filter_Condition.Not_In values -> Filter_Condition.Not_In (check_is_in_values "Not_In" values)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this check that the values do not contain a Column_Ref, because IIRC we have agreed that there is no sense to provide a column selector widget for this case - as it can be confusing. So I guess there is neither sense to resolve the Column_Refs here.

(The confusion is that normally operations work row-by-row but Is_In, when provided a column will look if the values on the left are contained anywhere within the column on the right - so it's a different logic.)

So I guessed that since we do not resolve the Column_Ref, we should not accept it either - otehrwise it would be compared by equality and just yield to unexpected results.

So I added this errror:
image

Column_Ref is not allowed in Is_In to avoid unexpected behavior. As opposed to other operations, which operate on a row-by-row basis when passed a column, Is_In looks at the whole contents of the passed collection - thus usually expecting a Vector. If you want to filter the elements checking if they are present anywhere in a passed column, you can pass a column by first getting it from the table using the at operator.

IMO it explains OK why by default this is not allowed and how the user can work around it if they know what they are doing (use at).

Please note that in normal flow, the user should never see this error - the widget for Is_In does not suggest selecting column references - it includes blank _ nodes instead waiting for input. The error will only show up if the user manually types in Column_Ref.Name "..." for this argument.

Comment on lines +1 to +9
from Standard.Base import all

## Reference to a column in a table.
type Column_Ref
## Reference to a column by name in a table.
Name name:Text

## Reference to a column by index in a table.
Index index:Integer
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved Column_Ref to a separate file, because I think it may be used in other contexts as well so it seemed sensible to have it in its own file.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Member Author

@jdunkerley jdunkerley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Radek this looks great.

Comment on lines +1 to +9
from Standard.Base import all

## Reference to a column in a table.
type Column_Ref
## Reference to a column by name in a table.
Name name:Text

## Reference to a column by index in a table.
Index index:Integer
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@radeusgd radeusgd added the CI: Ready to merge This PR is eligible for automatic merge label Oct 13, 2023
@mergify mergify bot merged commit fac9e7a into develop Oct 13, 2023
31 checks passed
@mergify mergify bot deleted the wip/jd/derive-column branch October 13, 2023 16:03
@enso-bot
Copy link

enso-bot bot commented Oct 16, 2023

Radosław Waśko reports a new STANDUP for the provided date (2023-10-13):

Progress: Created followup tasks. Cleanup; got the PR merged. Work on optimizing add row number. It should be finished by 2023-10-13.

Next Day: Next day I will be working on the #8055 task. Add another benchmark and see potential reasons for slowness, summarize. Get the Problem handling refactor merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants