-
Notifications
You must be signed in to change notification settings - Fork 323
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
Fix for Data Quality perf and issue other tweaks. #11711
Conversation
Note when sampled.
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.
Is it possible to add tests for any of the new functionality?
distribution/lib/Standard/Visualization/0.0.0-dev/src/Table/Visualization.enso
Outdated
Show resolved
Hide resolved
std-bits/table/src/main/java/org/enso/table/data/column/operation/CountUntrimmed.java
Show resolved
Hide resolved
Tests added for the table viz have been updated to cover the DB Table and sampled data quality indicator. |
std-bits/table/src/main/java/org/enso/table/data/column/operation/CountUntrimmed.java
Outdated
Show resolved
Hide resolved
std-bits/table/src/main/java/org/enso/table/data/column/storage/StringStorage.java
Outdated
Show resolved
Hide resolved
std-bits/table/src/main/java/org/enso/table/data/column/storage/StringStorage.java
Outdated
Show resolved
Hide resolved
PR comments.
std-bits/table/src/main/java/org/enso/table/data/column/operation/CountUntrimmed.java
Outdated
Show resolved
Hide resolved
std-bits/table/src/main/java/org/enso/table/data/column/storage/StringStorage.java
Outdated
Show resolved
Hide resolved
std-bits/table/src/main/java/org/enso/table/data/column/storage/StringStorage.java
Show resolved
Hide resolved
std-bits/table/src/main/java/org/enso/table/data/column/storage/StringStorage.java
Outdated
Show resolved
Hide resolved
std-bits/table/src/main/java/org/enso/table/data/column/operation/CountUntrimmed.java
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.
Looks good now, mostly some small nitpicks that I think may be worth adding but not necessary.
Only thing I'd like to fix before merging is the computation of countUntrimmed
if the cached future was cancelled. Currently it runs on background thread but tries to fetch the context - that is bound to fail. We should just compute it on main thread if the background one was cancelled and it is being requested right now - deferring to background blocks the main thread until background thread is free to execute a new job, which may theoretically be a pretty long time.
- Fix bug where `DB_Table` data quality indicators broke deserialization in the table viz. - Memorization of the untrimmed data quality indicator and move to it being an operation and column function. - If more than 10,000 rows then use a sample for untrimmed. - ALIASes for blank functions. - Fix for Snowflake drill down. - Bug fix for Long and Double columns with Nothings at end. (cherry picked from commit 85c8f76)
Pull Request Description
DB_Table
data quality indicators broke deserialization in the table viz.Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
Scala,
Java,
TypeScript,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
or the Snowflake database integration, a run of the Extra Tests has been scheduled.