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

HCK-7055 - Fix sonarlint reports #219

Merged
merged 2 commits into from
Jul 12, 2024
Merged

Conversation

bigorn0
Copy link
Contributor

@bigorn0 bigorn0 commented Jul 4, 2024

@@ -1321,6 +1304,7 @@ class Visitor extends HiveParserVisitor {
};
}

// Check that this function is identical to the visitCreateBloomfilterIndexMainStatement
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be assessed how you want to tackle that especially because semantically visiting the create and the drop statement share the exact same logic duplicated (not super obvious why because one could expect the must return different things...)

Copy link
Contributor

Choose a reason for hiding this comment

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

@bigorn0 We can move duplicated logic inside these two methods to a separate method in the visitor class, but having two entry points (visitDropBloomfilterIndexMainStatement and visitCreateBloomfilterIndexMainStatement) is required by ANTLR4 grammar - we should keep them.

Copy link
Contributor Author

@bigorn0 bigorn0 Jul 4, 2024

Choose a reason for hiding this comment

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

Yes it is indeed required, shouldn't we expect different information extracted et returned by the visitor function for create vs drop?

Copy link
Contributor

@VitaliiBedletskyi VitaliiBedletskyi Jul 4, 2024

Choose a reason for hiding this comment

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

Both methods return objects that represent BloomfilterIndex even if it's a DROP statement. As I know originally it was a requirement to parse such statements (DROP... statements) and reverse them in the Hackolade model as deactivated objects. But the BloomfilterIndexes doesn't have the isActivated property - so as a result we have the identical logic and it's expected for now I supose.

@@ -649,10 +642,6 @@ class Visitor extends HiveParserVisitor {
}
}

visitAlterStatementSuffixRename(ctx) {
Copy link
Contributor

Choose a reason for hiding this comment

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

deleted the wrong one 🙂 the last one declared takes precedence afaik, so this particular one was in use

@bigorn0 bigorn0 changed the title Fix sonarlint reports HCK-7055 - Fix sonarlint reports Jul 4, 2024
@bigorn0 bigorn0 closed this Jul 4, 2024
@bigorn0 bigorn0 reopened this Jul 4, 2024
@chulanovskyi-bs chulanovskyi-bs merged commit 2af2243 into develop Jul 12, 2024
@chulanovskyi-bs chulanovskyi-bs deleted the fix-sonar-reports branch July 12, 2024 13:34
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants