-
Notifications
You must be signed in to change notification settings - Fork 38
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
feat: SQLite support via JDBC #762
Conversation
40c46c3
to
3c27e60
Compare
7db43c9
to
492769b
Compare
db773cd
to
ba5ac05
Compare
The SQLite support finally got merge ready! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a comment. Other than that, LGTM. Please take a look when you have time!
String jdbcUrl = System.getProperty(PROP_JDBC_URL, DEFAULT_JDBC_URL); | ||
return jdbcUrl.startsWith("jdbc:sqlite:"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I missed this in the previous PR for some reason. Can you please add a method doing this in the JdbcEnv
class, and call the new method here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you! 🎉
private static final String PROP_JDBC_URL = "scalardb.jdbc.url"; | ||
private static final String DEFAULT_JDBC_URL = "jdbc:mysql://localhost:3306/"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please also delete these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I did it: d790e90
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thank you!
One thing, I just noticed this PR has docs/getting-started/sample.db
. Do we really need this file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thank you!
This is a base (huge) pull request to fully support SQLite.
See separate pull requests for code review.