Skip to content
This repository has been archived by the owner on Mar 2, 2019. It is now read-only.

Support drop table #17

Merged
merged 5 commits into from
Jun 30, 2015
Merged

Support drop table #17

merged 5 commits into from
Jun 30, 2015

Conversation

shadeven
Copy link
Contributor

@alexfu, please review the pull request. This adds a new feature to support DROP TABLE statement with optional IF EXISTS.

@alexfu
Copy link
Owner

alexfu commented Jun 29, 2015

Couple things...

  • There are many things a user can drop including tables (view, trigger, index, etc). I think it would be better suited and more extensible if we had a DropSegmentBuilder that adds the DROP keyword in and then has exposed functions to table, view, index, or trigger. Since the DROP statement are pretty much the same for all 4 (except for the name), we can just assign a type when calling through to the exposed table, view, index, or trigger function. Then in the build method, we would combine the all of the components (type, name, and the keyword DROP). In the end I think the query would look something like this...
String query = SQLiteQueryBuilder
  .drop()
  .table()
  .ifExists()
  .name("my_table")
  .toString()

String query = SQLiteQueryBuilder
  .drop()
  .index()
  .ifExists()
  .name("my_index")
  .toString()

This also matches the exact query language format for SQLite which I think should be preserved.

  • It looks like you have a tab spacing of 4, it should be 2 for this project.

@alexfu
Copy link
Owner

alexfu commented Jun 29, 2015

I actually think it might be acceptable to leave the name function out and roll it into the index() or table() calls, so it would look like...

String query = SQLiteQueryBuilder
  .drop()
  .index("my_index")
  .ifExists()
  .toString()

@shadeven
Copy link
Contributor Author

@alexfu, thanks for your feedback. I will make the changes tonight.
The indent settings seems revert back to 2 somehow 😕.

@shadeven
Copy link
Contributor Author

@alexfu, I just made the changes and pushed. Please have a look.

@shadeven
Copy link
Contributor Author

@alexfu, actually should I remove DropBuilder class, and let DropSegmentBuilder do all the work?
What do you think?

@shadeven
Copy link
Contributor Author

@alexfu, I made additional changes in 23f66e2. Please review when you have time.


@Override
public String build() {
return StringUtils.join(" ", "DROP", type, (ifExists ? "IF EXISTS" : ""), name);
Copy link
Owner

Choose a reason for hiding this comment

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

This will create an extra space if ifExists is false between the type and name.

@shadeven
Copy link
Contributor Author

You are up early, mate.
Good catch! Fixed in 54426a0.

@alexfu
Copy link
Owner

alexfu commented Jun 30, 2015

@shadeven looks good. I'll go ahead and merge.

alexfu added a commit that referenced this pull request Jun 30, 2015
@alexfu alexfu merged commit c631fcc into alexfu:develop Jun 30, 2015
@shadeven shadeven deleted the feature/drop-table branch June 30, 2015 23:42
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants