-
Notifications
You must be signed in to change notification settings - Fork 326
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
Expanding Derived Columns and Expression Syntax #3782
Conversation
dd1eecb
to
4b52532
Compare
af9f9aa
to
d4fcc24
Compare
8bca32f
to
ea997e4
Compare
63cef2c
to
06fdc78
Compare
distribution/lib/Standard/Database/0.0.0-dev/src/Data/Column.enso
Outdated
Show resolved
Hide resolved
distribution/lib/Standard/Database/0.0.0-dev/src/Data/SQL_Statement.enso
Outdated
Show resolved
Hide resolved
distribution/lib/Standard/Database/0.0.0-dev/src/Data/Table.enso
Outdated
Show resolved
Hide resolved
distribution/lib/Standard/Database/0.0.0-dev/src/Data/Table.enso
Outdated
Show resolved
Hide resolved
distribution/lib/Standard/Table/0.0.0-dev/src/Data/Expression.enso
Outdated
Show resolved
Hide resolved
_ : Text -> | ||
table_at = self.at column | ||
if table_at.is_error.not then self.filter table_at filter on_problems else | ||
expression = self.evaluate column | ||
if expression.is_error.not then self.filter expression filter on_problems else | ||
pick_error = expression.catch Expression_Error.Syntax_Error (_->table_at) | ||
on_problems.handle_errors pick_error fallback=self |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems exactly the same as in DB, right? I'm wondering if we should extract it to some Table_Helpers
to DRY. Not entirely sure though.
distribution/lib/Standard/Table/0.0.0-dev/src/Data/Expression.enso
Outdated
Show resolved
Hide resolved
9c07170
to
cc37a0e
Compare
std-bits/table/src/main/java/org/enso/table/data/column/storage/LongStorage.java
Outdated
Show resolved
Hide resolved
...bits/table/src/main/java/org/enso/table/data/column/operation/map/numeric/LongNumericOp.java
Show resolved
Hide resolved
distribution/lib/Standard/Database/0.0.0-dev/src/Data/Column.enso
Outdated
Show resolved
Hide resolved
distribution/lib/Standard/Database/0.0.0-dev/src/Data/SQL_Statement.enso
Outdated
Show resolved
Hide resolved
distribution/lib/Standard/Database/0.0.0-dev/src/Data/Table.enso
Outdated
Show resolved
Hide resolved
distribution/lib/Standard/Table/0.0.0-dev/src/Data/Expression.enso
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The results are really amazing! 🎉
In general it looks good to me, I have just some style comments.
I'm thinking if it would be worth to add some more comments explaining how ExpressionVisitorImpl
is meant to work - I think such documentation will be helpful in maintaining it in the future.
std-bits/table/src/main/java/org/enso/table/expressions/ExpressionVisitorImpl.java
Show resolved
Hide resolved
return metaObject != null && metaObject.asHostObject() instanceof Class<?> | ||
? makeConstantColumn.apply(value) | ||
: value; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if I understand this condition - I understand that if an object comes from Java, we pass it over to makeConstantColumn
, but otherwise we just pass it as-is. Why is that so? The name wrapAsColumn
is not clear enough to me to say what should be expected here.
Can we add some comment to this function explaining what it is meant to do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After looking forward at the code I think I now understand what is the meaning, but now I'm afraid of this logic.
It will make a constant column from any Java value but pass Enso values as-is. I guess for now it is OK, but if in the future we allow somehow in the expression to take hold of a singleton Enso value, this part of the logic will break - we'd expect it to wrap the value to a column, but it would be passed as-is instead.
Can we change the logic to detect the column? Maybe we can look at the MetaObject and see if it matches the moduleName
(when Jaroslav's changes with MetaObjects for Atoms are integrated).
If not, let's at least add a comment warning that we may need to be careful in case of any changes.
std-bits/table/src/main/java/org/enso/table/expressions/ExpressionVisitorImpl.java
Outdated
Show resolved
Hide resolved
773371d
to
f40dc73
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I understand the reasons why this DSL is being created, I am sad to see the Enso language itself was not found capable enough to provide similar (and better) user experience. My hope is that it is just a temporary state and the Enso language improves to provide good enough support to turn this DSL into an internal DSL.
If you share a similar hope, then it'd make sense to package this DSL as a separate library that extends the Standard.Table
one. That would allow for a smoother replacement with an internal DSL in the future.
expression_test "[B] / [A]" [1, 0.75, 0.8333333333333334, 1, 1.2] | ||
expression_test "[A] ^ [B]" [1, 2.8284271247461903, 15.588457268119896, 256, 15625] | ||
expression_test "[A] % [B]" [0, 0.5, 0.5, 0, 5] | ||
expression_test "[A] + [B] + [A]" [3, 5.5, 8.5, 12, 16] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ten years ago I had a DSL vs. API design shootout with a friend at a conference. We had to write our notes down to even understand each other. Given our classification, what we have here is an external DSL with its own parser embedded in an Enso string.
A string based DSL approach will increase the complexity of our tooling. We will not be able to benefit from Enso type system, use Enso suggestions DB, etc. We will have to find ways for the IDE to understand this DSL syntax.
Given the sole purpose of Enso was to work effectively with tables, this feels as a side step. A recognition that Enso isn't good enough at manipulating tables yet. I sincerely hope that over the time Enso improves and provides good enough framework for dealing with tables as well.
Ideally we should create an internal DSL, reuse the Enso parser, intermix the DSL in between other Enso constructs. Make sure the Enso type system straightens the DSL and the DSL is 1st class citizen in the Enso code and can be naturally manipulated by the IDE.
e623bc0
to
5db850f
Compare
Allow up to nanoseconds in Time and DateTime. Handle unparsable date/time as Syntax errors.
Some more PR comments.
014a4a5
to
448e514
Compare
Pull Request Description
set
andfilter
on the Database and InMemoryTable
.aggregate
on the Database and InMemoryTable
.sum
,max
,min
andmean
) fromColumn
types.Column
+
operator to do concatenation (||
) when text types.^
to bothColumn
types.iif
to allow for columns to be passed forwhen_true
andwhen_false
parameters.is_present
to databaseColumn
type.coalesce
,min
andmax
functions to bothColumn
types performing row based operation.Date
,Time_Of_Day
andDate_Time
constants in database.read
method to InMemoryColumn
returningself
(or a slice).Important Notes
SQL_Type
.LongNumericOp
where it was always casting to a double.head
from InMemory Table (still hasfirst
method).Checklist
Please include the following checklist in your PR:
Scala,
Java,
and
Rust
style guides.
./run ide build
and./run ide watch
.