Skip to content

Validate view config in engine, fix computed column state bugs#1272

Merged
texodus merged 5 commits intomasterfrom
validate-view
Jan 7, 2021
Merged

Validate view config in engine, fix computed column state bugs#1272
texodus merged 5 commits intomasterfrom
validate-view

Conversation

@sc1f
Copy link
Contributor

@sc1f sc1f commented Jan 4, 2021

This PR makes several bug-fixes on the View creation and computed column features of the Perspective engine.

Most notably, it introduces full validation of view configurations for invalid columns, i.e. a column that is not present in the table. While this is not an issue for most Perspective users, restoring a configuration that has invalid columns, for example, can cause issues that were previously difficult to debug. Now, all configuration options on the view are validated, and if an invalid column is specified, abort() is called in Javascript or a PerspectiveCppException will be thrown in Python with a specific error message detailing the invalid column name and where it was found.

Additionally, this PR fixes several edge cases with computed column state:

  • Previously, one could create computed columns that had the same name as a "table column", i.e. column on the underlying table. Now, this will error in the computed columns UI on perspective-viewer, and the engine will abort() or throw PerspectiveCppException if attempted directly through the API.
  • Previously, one could create computed columns that had the same name as another computed column, but retype it (i.e. replacing a + b as c with uppercase(name) as c). Now, this will error in the computed columns UI, and the engine will abort() or throw PerspectiveCppException.
  • Previously, if multiple views were created at the same time with shared computed columns, deletion of any one of the views would cause the shared column to no longer calculate on subsequent update cycles, as view deletion also removes the view's computed columns from listening to further updates. Now, multiple views can share the same computed columns and as long as one view is in scope containing the shared column, the shared column will continue to correctly update.
    • This is/has not been an error on perspective-viewer, as the Viewer keeps track of one view at a time and does not simultaneously create multiple views, but it is a reproducible edge case that is now fixed.

Finally, more granular benchmarks for computed columns have been added to better gauge performance.

Edit: the additional complexity introduced with patching the shared-dependency issue has been removed from this PR, and the issue will be handled with additional feature/debt relief work that is part of building out the computed columns feature.

@sc1f sc1f added internal Internal refactoring and code quality improvement JS Python enhancement Feature requests or improvements labels Jan 4, 2021
Copy link
Member

@texodus texodus left a comment

Choose a reason for hiding this comment

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

Let's discuss m_computed_columns_count a bit before merging - it is likely worth it to fix this bug, but we need a concrete plan to address the temporary state management problem wrt Computer Column accounting.

} else {
m_computed_columns.erase(name);
m_computed_columns_count.erase(name);
}
Copy link
Member

Choose a reason for hiding this comment

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

The complexity this introduces may not be worth it IMO - e.g., another case that will trivially break is duplicate column name dependencies in separate viewers. We've discussed removing intermediate columns entirely from engine process() - I propose this may be an easier approach at this point than properly modeling all caveats of the intermediate column dependency system as currently written.


// Throw an exception if the computation is invalid - the UI will
// prevent users from saving invalidly-typed computations.
if (computation.m_name == INVALID_COMPUTED_FUNCTION) {
Copy link
Member

Choose a reason for hiding this comment

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

INVALID_COMPUTED_FUNCTION magic values seems to indicate to me this error should occur in get_computation(), since we're already using ABORT() for error messages?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The function returns INVALID_COMPUTED_FUNCTION instead of abort so that the calling function can print a verbose error message, but as discussed offline we should get rid of the verbose error messages along with other refactors in a debt relief task.

if (skip) {
// this column depends on a column that does not exist, so it
// does not need to be typechecked.
continue;
Copy link
Member

Choose a reason for hiding this comment

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

Why though? With the removal protection also added in this PR - this case is an error, the column data may even be stale if for some reason it exists without its dependents?

Copy link
Member

@texodus texodus 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! Thanks for the PR!

@texodus texodus merged commit b4ad2bd into master Jan 7, 2021
@texodus texodus deleted the validate-view branch January 7, 2021 05:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement Feature requests or improvements internal Internal refactoring and code quality improvement JS Python

Development

Successfully merging this pull request may close these issues.

2 participants