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

Adding new Date/Time operations (-, date_add, date_diff, date_part) #7221

Merged
merged 58 commits into from
Jul 13, 2023

Conversation

radeusgd
Copy link
Member

@radeusgd radeusgd commented Jul 5, 2023

Pull Request Description

  • Adds Column.date_diff for computing date/time difference as integer multiply of some unit.
  • Adds Column.date_add for shifting date/time by a unit.
  • Adds Column.date_part for extracting various parts of the date/time value as integer.
  • Adds widgets for the 3 methods above whose content depends on the column value type.
  • Adds shorthands: Column.hour, Column.minute and Column.second to extract these date parts.
  • Extends Time_Period with support for milli-, micro- and nano- seconds; and adapts functions taking Time_Period to support these wherever possible.

Important Notes

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.

@radeusgd radeusgd force-pushed the wip/radeusgd/column-date-ops branch 3 times, most recently from f8a5f6a to 4a14343 Compare July 7, 2023 16:21
@radeusgd radeusgd self-assigned this Jul 7, 2023
`Nothing` for most operations, but some operations that need to be
parametrized by additional settings can use this field to pass that
information to the code generator.
Operation (kind : Text) (expressions : Vector SQL_Expression) (metadata : Any | Nothing = Nothing)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we have a more restrictive type than Any | Nothing? Such as Row_Number_Metadata | Date_Period_Metadata? Or do you think there will be too many?

Copy link
Member Author

Choose a reason for hiding this comment

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

We could probably.

I'm not sure if this gives us much benefit - since the 'right' type depends on the particular kind anyway, this will not be checking much. We definitely want stronger typing in the new IR, but with this one which is rather dynamic, I'd stick to it I think.

Ideally we should have one parent 'class' or interface for the metadata, but due to lack of inheritance/interfaces we cannot have it. The sum type could work, but I'm afraid it could become long - ideally we should define an alias in the file with metadata and only reference the alias here - but type aliases are not supported yet.

@radeusgd radeusgd force-pushed the wip/radeusgd/column-date-ops branch 4 times, most recently from e0f3a84 to 7bb51df Compare July 11, 2023 10:19
@radeusgd radeusgd changed the title Adding new Date/Time operations (-, date_add, date_diff) Adding new Date/Time operations (-, date_add, date_diff, date_part) Jul 11, 2023
@radeusgd
Copy link
Member Author

Widgets depending on the type:
image
image
image
image
image

If the Date_Period/Time_Period is invalid for a given type, an error is reported:
image

@radeusgd
Copy link
Member Author

I wasn't sure what to display if the input column type is invalid (i.e. Integer not date/time).
Ideally, we want an error there, but IIRC we cannot easily trigger it from the widget - and the function will not error until all non-default arguments are provided.

So what I did is I 'sneak' the error message into the widget:
image
After all arguments are provided, the error appears on the node as well:
image
image
image

I'm however not sure if this is the right approach. It seems good to notify the user of the error as soon as possible, but that seems a bit abusing the architecture - the widget is probably not the right place to display errors/warnings.

However, I definitely did not want to display an empty selector there - that would be even worse.

As a viable alternative, I think we can just display all available options (i.e. the same as for Date_Time) when the type is invalid - so the user can choose whatever and then possibly fix the type.

Which one of these two you think is better?

  1. Display all possible choices when type is wrong.
  2. Display the error message like shown on screenshots.

@radeusgd radeusgd marked this pull request as ready for review July 11, 2023 12:25
Copy link
Member

@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.

Looks good - minor comment but great to have this in.

@radeusgd radeusgd force-pushed the wip/radeusgd/column-date-ops branch from 8dfbc07 to bc918df Compare July 12, 2023 12:29
@radeusgd radeusgd added the CI: Ready to merge This PR is eligible for automatic merge label Jul 12, 2023
@mergify mergify bot merged commit ca68dd9 into develop Jul 13, 2023
@mergify mergify bot deleted the wip/radeusgd/column-date-ops branch July 13, 2023 12:56
mergify bot pushed a commit that referenced this pull request Jul 13, 2023
…values. (#7273)

Followup of #7221, adding `date_diff`, `date_add` and `date_part` to scalar Enso date-time values.
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.

3 participants