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

MODE-2350 OracleDdlParser update for multi-op ALTER TABLE #1303

Closed
wants to merge 1 commit into from
Closed

MODE-2350 OracleDdlParser update for multi-op ALTER TABLE #1303

wants to merge 1 commit into from

Conversation

freakolowsky
Copy link
Contributor

Method parseAlterTableStatement now supports basic chained ADD and/or
MODIFY commands. The existing functionality should be the same (ran
tests without issues).

Will do some more coding later to add complete column definitions as
ALTER TABLE MODIFY currently only detects changes that include datatype
modifications.

  • modified parseAlterTableStatement
  • modified parseColumns and overloaded for compatibility
  • created Oracle local parseColumnDefinition
  • hopefuly not broken things (... much)

@hchiorean
Copy link
Member

looks fine, provided there are no regressions.
@rhauch: I guess we'd a new JIRA enhancement for this ?

@rhauch
Copy link
Contributor

rhauch commented Oct 23, 2014

Yes, we need a new enhancement JIRA that adequately explains the changes in terms of the DDL expressions to support. Then please update the commit message (using git amend is easiest) to include the JIRA number at the front (e.g., MODE-5678).

It'd probably be good to add a few more unit tests that cover not just the new capability, but also handle some failure cases, such as when the new expressions are formed incorrectly.

@hchiorean
Copy link
Member

@freakolowsky: can you pls open the enhancement JIRA, tx.

Method parseAlterTableStatement now supports basic chained ADD and/or
MODIFY commands. The existing functionality should be the same (ran
tests without issues).

Will do some more coding later to add complete column definitions as
ALTER TABLE MODIFY currently only detects changes that include datatype
modifications.

 * modified parseAlterTableStatement

 * modified parseColumns and overloaded for compatibility

 * created Oracle local parseColumnDefinition

 * hopefuly not broken things (... much)
@freakolowsky
Copy link
Contributor Author

Like this? ... sry, but i'm new in this part of town :D

@hchiorean
Copy link
Member

@freakolowsky: yes, that's pretty close. tx.

@freakolowsky
Copy link
Contributor Author

oh yes ... @rhauch incorrectly formed expression should already be handled by existing tests, this mod only adds support for ADD/MODIFY repeats

@hchiorean: what am i missing? if i'll poking around this code i don't want to piss pplz off on the first try :D

@hchiorean
Copy link
Member

@freakolowsky: in the context of the changes (just supporting multiple ADD and MODIFY statements within ALTER TABLE) not sure what rhauch meant by

handle some failure cases, such as when the new expressions are formed incorrectly.

@rhauch ^^ ?

@rhauch
Copy link
Contributor

rhauch commented Oct 24, 2014

@hchiorean, @freakolowsky: My comment about testing improper expressions is simply to verify that the parser correctly generates a useful exception/problem when an expression is poorly formed. Doesn't need to be anything fancy or really that many. IOW, the repeating ADD and MODIFY statements are what's new, so try to test a couple of expressions that use them incorrectly just to make sure that a decent error/problem is reported.

@rhauch
Copy link
Contributor

rhauch commented Oct 24, 2014

@freakolowsky: Overall, this is stellar. Thank you for the contribution! Yeah, there are a couple of minor things we wanted changed, mostly just to follow our change process (file an issue, make sure the PR references the issue, make sure the issue references the PR, etc.).

You also wrote:

if i'll poking around this code i don't want to piss pplz off on the first try :D

No worries there! In fact, we're very grateful for the improvement! Keep up the good work!

@freakolowsky
Copy link
Contributor Author

@rhauch np ... this chunk of modeshape project saved me from having to write a DDL parser from scratch which i need for another project

@hchiorean
Copy link
Member

@freakolowsky: based on the previous comments, I'd say it's safe to amend this & add a couple of more tests with invalid ADD & MODIFY sequences and after that I'll merge this.

@freakolowsky
Copy link
Contributor Author

Will try to do it over the weekend or next week ...

@hchiorean
Copy link
Member

@freakolowsky: any update on this ?

@rhauch
Copy link
Contributor

rhauch commented Nov 24, 2014

@freakolowsky Do you think you might update the PR soon?

@freakolowsky
Copy link
Contributor Author

sry 4 the delay ... had some personal issues (father got cancer) and had to spend a lot of time out of the office. Will update asap. Don't give up on me yet :D

@rhauch
Copy link
Contributor

rhauch commented Nov 26, 2014

@freakolowsky, no worries. Thanks for the reply, and I'm very sorry to hear about your father. We can keep this PR open until you have a chance to make the changes.

@hchiorean hchiorean changed the title OracleDdlParser update for multi-op ALTER TABLE MODE-2350 OracleDdlParser update for multi-op ALTER TABLE Jan 29, 2015
@hchiorean
Copy link
Member

superseded by #1383

@hchiorean hchiorean closed this Feb 2, 2015
@freakolowsky freakolowsky deleted the oracle-alter-table-parser branch May 12, 2015 12:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants