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

HPCC-20590 Suppress all semantic errors when syntax checking #11735

Merged
merged 2 commits into from Oct 2, 2018

Conversation

Projects
None yet
4 participants
@shamser
Copy link
Contributor

commented Sep 21, 2018

Signed-off-by: Shamser Ahmed shamser.ahmed@lexisnexis.co.uk

Type of change:

  • This change is a bug fix (non-breaking change which fixes an issue).
  • This change is a new feature (non-breaking change which adds functionality).
  • This change improves the code (refactor or other change that does not change the functionality)
  • This change fixes warnings (the fix does not alter the functionality or the generated code)
  • This change is a breaking change (fix or feature that will cause existing behavior to change).
  • This change alters the query API (existing queries will have to be recompiled)

Checklist:

  • My code follows the code style of this project.
    • My code does not create any new warnings from compiler, build system, or lint.
  • The commit message is properly formatted and free of typos.
    • The commit message title makes sense in a changelog, by itself.
    • The commit is signed.
  • My change requires a change to the documentation.
    • I have updated the documentation accordingly, or...
    • I have created a JIRA ticket to update the documentation.
    • Any new interfaces or exported functions are appropriately commented.
  • I have read the CONTRIBUTORS document.
  • The change has been fully tested:
    • I have added tests to cover my changes.
    • All new and existing tests passed.
    • I have checked that this change does not introduce memory leaks.
    • I have used Valgrind or similar tools to check for potential issues.
  • I have given due consideration to all of the following potential concerns:
    • Scalability
    • Performance
    • Security
    • Thread-safety
    • Premature optimization
    • Existing deployed queries will not be broken
    • This change fixes the problem, not just the symptom
    • The target branch of this pull request is appropriate for such a change.
  • There are no similar instances of the same problem that should be addressed
    • I have addressed them here
    • I have raised JIRA issues to address them separately
  • This is a user interface / front-end modification
    • I have tested my changes in multiple modern browsers
    • The component(s) render as expected

Testing:

HPCC-20590 Suppress all semantic errors when syntax checking
Signed-off-by: Shamser Ahmed <shamser.ahmed@lexisnexis.co.uk>
@hpcc-jirabot

This comment has been minimized.

Copy link

commented Sep 21, 2018

@shamser

This comment has been minimized.

Copy link
Contributor Author

commented Sep 21, 2018

@ghalliday Please can you review

@ghalliday
Copy link
Member

left a comment

@shamser minor comments - main one is that I'm not sure if this test is quite correct.

@@ -1022,6 +1022,7 @@ class HQL_API HqlLookupContext
inline bool syntaxChecking() const { return parseCtx.isSyntaxChecking(); }
inline bool regenerateCache() const { return parseCtx.regenerateCache; }
inline bool hasCacheLocation() const { return parseCtx.hasCacheLocation();}
inline bool syntaxCheckingUsingCache() const { return parseCtx.isSyntaxChecking() && parseCtx.hasCacheLocation(); }

This comment has been minimized.

Copy link
@ghalliday

ghalliday Sep 24, 2018

Member

discussion: I would suggest calling this suppressSemanticErrors(). The reason is that it is then self documenting at the point of call. if (!lookupCtx.suppressSemantciErrors() || ...)
The reason for raining it is that I'm not sure if this condition is quite right. For instance when running the regression suite to test the syntax checking you also need to suppress the errors. I think it would currently be !isSyntaxchecking && !ignoreCache.

This comment has been minimized.

Copy link
@shamser

shamser Sep 25, 2018

Author Contributor

I've removed this function because after commoning up the code below this function is used just once.

@@ -6171,8 +6171,11 @@ void HqlGram::reportErrorUnexpectedX(const attribute& errpos, IAtom * unexpected

void HqlGram::doReportWarning(WarnErrorCategory category, int warnNo, const char *msg, const char *filename, int lineno, int column, int pos)
{
Owned<IError> error = createError(category, queryDefaultSeverity(category), warnNo, msg, filename, lineno, column, pos);
report(error);
if (!lookupCtx.syntaxCheckingUsingCache() || category==CategorySyntax || category==CategoryError)

This comment has been minimized.

Copy link
@ghalliday

ghalliday Sep 24, 2018

Member

minor: Consider including in a function since this code is repeated 5 times. Adding a function can also aid readability. E.g. if (includeError(lookupCtx, category))

@ghalliday

This comment has been minimized.

Copy link
Member

commented Sep 24, 2018

It could alternatively be that the test in parseAttribribute hqlexpr.cpp(12338) should be modified.

@shamser

This comment has been minimized.

Copy link
Contributor Author

commented Sep 25, 2018

It could alternatively be that the test in parseAttribribute hqlexpr.cpp(12338) should be modified.

Do you mean hqlgram2.cpp(12338)?

HPCC-20590 Improvements following review comments
Signed-off-by: Shamser Ahmed <shamser.ahmed@lexisnexis.co.uk>

@shamser shamser force-pushed the shamser:issue20590 branch from c8447b8 to 0eabdca Sep 25, 2018

@@ -1022,7 +1022,6 @@ class HQL_API HqlLookupContext
inline bool syntaxChecking() const { return parseCtx.isSyntaxChecking(); }
inline bool regenerateCache() const { return parseCtx.regenerateCache; }
inline bool hasCacheLocation() const { return parseCtx.hasCacheLocation();}
inline bool syntaxCheckingUsingCache() const { return parseCtx.isSyntaxChecking() && parseCtx.hasCacheLocation(); }

This comment has been minimized.

Copy link
@shamser

shamser Sep 25, 2018

Author Contributor

I've removed this function because after commoning up the code below this function is used just once.

@shamser

This comment has been minimized.

Copy link
Contributor Author

commented Sep 25, 2018

@ghalliday Please can you review.

@HPCCSmoketest

This comment has been minimized.

Copy link
Contributor

commented Sep 25, 2018

Automated Smoketest:
OS: centos 7.4.1708 (Linux 3.10.0-327.28.3.el7.x86_64)
Sha: 0eabdca
Build: success
Install hpccsystems-platform-community_7.0.0-rc2.el7.x86_64.rpm
HPCC Start: OK

Unit tests result:

Test total passed failed errors timeout
unittest 94 94 0 0 0
wutoolTest(Dali) 19 19 0 0 0
wutoolTest(Cassandra) 19 19 0 0 0

Regression test result:

phase total pass fail
setup (hthor) 11 11 0
setup (thor) 11 11 0
setup (roxie) 11 11 0
test (hthor) 806 806 0
test (thor) 730 730 0
test (roxie) 879 879 0

HPCC Stop: OK
HPCC Uninstall: OK
Time stats:

Prep time Build time Package time Install time Start time Test time Stop time Summary
19 sec (00:00:19) 187 sec (00:03:07) 95 sec (00:01:35) 20 sec (00:00:20) 43 sec (00:00:43) 1454 sec (00:24:14) 18 sec (00:00:18) 1836 sec (00:30:36)
@ghalliday
Copy link
Member

left a comment

Forgot to submit this. I'll come back to it today and check again

@@ -6169,10 +6169,26 @@ void HqlGram::reportErrorUnexpectedX(const attribute& errpos, IAtom * unexpected
reportError(ERR_UNEXPECTED_ATTRX, errpos, "Unexpected attribute %s", str(unexpected));
}

static bool includeError(HqlLookupContext & ctx, WarnErrorCategory category)
{
if (ctx.syntaxChecking() && !ctx.ignoreCache())

This comment has been minimized.

Copy link
@ghalliday

ghalliday Oct 2, 2018

Member

I think the PR is ok, but this is the line I am debating about. And my debate is more about the corresponding call in hqlgram2.cpp(12353)

I am trying to work out what options can be used to regression test the private suite. I think it requires an empty --metacache, syntax checklng and ignore cache to be not set.

@ghalliday ghalliday merged commit b81cbd3 into hpcc-systems:master Oct 2, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.