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

HHH-14033 - SQL script parsing problem with multi-line comments #3414

Merged
merged 1 commit into from
May 20, 2020

Conversation

sebersole
Copy link
Member

@sebersole sebersole commented May 19, 2020

  • Better handling of multi-line comments
  • Restructured some internal classes to consolidate packages
  • Added "system"-style SchemaToolingLogging

https://hibernate.atlassian.net/browse/HHH-14033

- Better handling of multi-line comments
- Restructured some internal classes to consolidate packages
- Added "system"-style SchemaToolingLogging
@sebersole sebersole requested review from dreab8 and Sanne May 19, 2020 17:39
@sebersole
Copy link
Member Author

Consider specifically whether y'all think it is appropriate to switch the exception type for the missing ; error

@dreab8
Copy link
Contributor

dreab8 commented May 20, 2020

Hi Steve, the specific message was introduce to fix quarkusio/quarkus#3817, but I do not think that changing the exception type can cause any problem.

Copy link
Contributor

@dreab8 dreab8 left a comment

Choose a reason for hiding this comment

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

it looks very good to me 👍

@@ -203,6 +203,9 @@ xjc {
}
}

generateGrammarSource {
arguments += "-traceParser"
Copy link
Member

Choose a reason for hiding this comment

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

Is this going to affect the regular HQL parser as well?

Specifically I'm wondering about performance impact, if this is to help during development maybe it shoduld be commented out when done with debugging?

Copy link
Member Author

Choose a reason for hiding this comment

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

The impact - if you look in the generated parsers, you will now see Antlr has added traceIn/traceOut calls at the start and end of each rule method. I use that as hooks to debug "parse tree info". This is extremely useful information for debugging parse problems; unless we can demonstrate that it is a performance problem I'd prefer to leave these as-is.

Copy link
Member Author

Choose a reason for hiding this comment

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

Its a good point though in that the default impl Antlr injects is to use sysout. For HQL and for SQL script parsing I've overridden these to use logging. I'll add the same override for the entity-graph parser and really the impact overall should be minimal

@sebersole
Copy link
Member Author

sebersole commented May 20, 2020

the specific message was introduce to fix quarkusio/quarkus#3817

Right - I figured. Which is why I kept the message unchanged

@sebersole
Copy link
Member Author

sebersole commented May 20, 2020

Going to push an additional commit. If y'all agree I'll just push this to master

@dreab8
Copy link
Contributor

dreab8 commented May 20, 2020

Going to push an additional commit. If y'all agree I'll just push this to master

@sebersole it is fine to me

@sebersole sebersole merged commit b658e90 into hibernate:master May 20, 2020
@sebersole sebersole deleted the HHH-14033 branch May 20, 2020 12:04
@Sanne
Copy link
Member

Sanne commented May 20, 2020

Going to push an additional commit. If y'all agree I'll just push this to master

sure!

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