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

Simplify ColumnExpressionValidator #2650

Merged

Conversation

devinrsmith
Copy link
Member

No description provided.

@devinrsmith devinrsmith self-assigned this Jul 19, 2022
@devinrsmith devinrsmith force-pushed the simplify-column-expr-validator branch 2 times, most recently from b7f7212 to 5567c41 Compare July 20, 2022 19:25
@devinrsmith devinrsmith force-pushed the simplify-column-expr-validator branch from 5567c41 to 8a15bc9 Compare July 21, 2022 15:21
@devinrsmith devinrsmith marked this pull request as ready for review July 21, 2022 17:44
@devinrsmith devinrsmith added this to the Jul 2022 milestone Jul 21, 2022
@devinrsmith devinrsmith requested a review from rcaudy July 21, 2022 17:45
@@ -129,7 +129,7 @@ public String toString() {
}

public SelectColumn getRealColumn() {
return realColumn;
return Require.neqNull(realColumn, "realColumn");
Copy link
Member

Choose a reason for hiding this comment

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

There's a bit of inconsistency in this. Some methods check it, others don't, and none are going to give an appropriate error message. Maybe the other accesses should thread through getRealColumn(), and maybe it should have a better validation, like:

        if (realColumn == null) {
            throw new IllegalStateException(
                    "Real column is not available until this SwitchColumn is initialized; ensure that initInputs or initDef has been called first"
            );
        }

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea.

@devinrsmith devinrsmith requested a review from rcaudy July 21, 2022 21:48
@devinrsmith devinrsmith merged commit 2f00160 into deephaven:main Jul 22, 2022
@devinrsmith devinrsmith deleted the simplify-column-expr-validator branch July 22, 2022 22:03
@github-actions github-actions bot locked and limited conversation to collaborators Jul 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants