Again trying to complete SQLDroidDatabaseMetaData#39
Again trying to complete SQLDroidDatabaseMetaData#39nlmarco wants to merge 3 commits intoSQLDroid:masterfrom
Conversation
decompiled corresponding class class from https://github.com/plingpling/Asper/blob/master/source/libs/sqlite.jar as mentioned on http://stackoverflow.com/questions/11933387/sqldroid-vs-android-internal-jdbc-driver
This time copying from https://bitbucket.org/xerial/sqlite-jdbc (which is Apache licence).
|
I didn't look at your code, but the original looks good. Would it make sense to take that as-is and create a SQLDroidDatabaseMetaData class that adds the extra method and overrides things like getDriverName? If we keep that metadata file unchanged, we can pull future enhancements There is some, fairly minimal, test code that (IIRC) tests some of the Jim On 04/23/2014 07:09 AM, nlmarco wrote:
Jim Redman |
|
Hi Jim, first of all, I'm very sorry for replying so late. I was extremely busy with many things and didn't have time for SQLDroid before. I recommend to use things as they are. Most importantly, because we had to fix a few bugs in the original code from Xerial. It would be cool, of course, to commit them there, too, but our time is limited. Thus, I cannot guarantee that we'll be able to do so. In the long run the two versions of the class (I mean Xerial / SQLDroid) are likely converging, anyway. IMHO it's only a question of time that Xerial will add the missing methods for the new JDBC. Additionally, I didn't properly implement all of these new methods (don't remember, if I even implemented any of them properly or just added empty stubs). Therefore, it's better for any person merging code in the future, if its the same class. This way, e.g. an accidentally added duplicate method is easier found (by the compiler) than if our implementation in the sub-class silently overrides the new, proper implementation in the super-class. My colleague improved many more things and will create a new pull request later today (or maybe tomorrow). Btw. I saw that there is an error about "Travis CI" having failed. It failed becaus it didn't find the "android" command. I guess this is some problem in the build-system and unrelated to my changes (in this pull request). Best regards, Marco :-) |
|
Hi all! This PR is pretty old now. Is it still viable? |
|
Hi @nlmarco ! Do you still want to work on this PR? If not, we'll close it. We still want |
|
Hi donv, sorry, but I don't understand. I thought that the status "Merged" shown above means that our pull request was already accepted into the official codebase - on 2014-09-04. So why should I "still want to work on this PR"? We fixed SQLDroidDatabaseMetaData so that it works with http://datanucleus.org - which was what we needed. IIRC, we have even done quite a bit more than just this - i.e. implemented more methods than needed. As already stated before, this is due to us taking over code from xerial. Unfortunately, this official code was partially incomplete / buggy, and that's why we fixed quite a few methods - and even re-implemented some code completely. Of course, we could only fix bugs that we encountered. DataNucleus uses quite a lot of the methods available, but not all of them. Hence, I'm not sure what exactly is working and what not. But I know for sure that our adapted version of the SQLDroid-JDBC-driver is far more complete and more correct than the official SQLite-JDBC-driver from xerial. And I know, too, that our work made SQLDroid complete enough to work with the best ORM available - DataNucleus -, which needs to dynamically determine the current database structure (as it creates/updates the schema to fit the Java code). SQLDroid might work with other ORMs, too, because all of them have the same task to do - query/create/update the database schema dynamically. Having explained all this, please I'd like to ask the following questions to clarify what exactly you want me to do:
Best regards, Marco :-) |
|
WHAT A PITY! I just cloned both repositories - the official SQLDroid and the one we forked 2 years ago. And NO, NOTHING was merged. This is really a pity. I seriously wonder. Why didn't you accept our pull request? |
# Conflicts: # .gitignore # src/main/java/org/sqldroid/SQLDroidDatabaseMetaData.java
|
OK. I give it yet another try: I just merged from upstream and resubmit my pull request hereby. I reverted the changes to the .cvsignore (and took over the version from upstream 1-to-1), but I threw away the upstream-version of SQLDroidDatabaseMetaData - it's now my version again. ...and btw. I finally understood what this "Merged" above means: It's a referenced issue. |
|
Taking a look at the SQLDroidDatabaseMetadata class in the pull request, it seems like this has a complete different heritage from the one currently in SQLDroid. The order of the methods is different, so the diff is hard to understand. It is hard to see what impact the pull request will have. It will risk regressions and it's hard to see specifically which methods it implements or improves. I found several regressions in terms of #32. Looking at the possible differences, the list of prepared statements is longer for the pull request. Looking at getTables, getColumnsTblName, getTypeInfo, it looks like the implementation is different, but doesn't offer any new features. |
|
I already explained that my SQLDroidDatabaseMetadata version originates from Xerial's official JDBC driver. I decided to throw SQLDroid's version away, completely. The reasons are simple:
I just checked #32 and I've to say that these are really unimportant and minor changes compared to the actual functionality & bugfixes I added. I'd be willing to replace the non-implemented method stubs accordingly, but given the fact that you seem reluctant to accept my pull request (which took quite some time already), I'm now reluctant to invest even more time - likely in vain. I don't know what work was done to SQLDroid's version of SQLDroidDatabaseMetadata after deciding not to accept my pull request 2 years ago (why?). And since I currently do not have a workspace using DataNucleus with SQLDroid, I cannot re-test, now. However, I can tell you that the original methods of SQLDroid's SQLDroidDatabaseMetadata returned incomplete and even wrong data. I remember that there was null in result-set-cells that MUST contain real entries and that especially the foreign-key-information was wrong (I think the direction (from/to table) of the returned foreign keys was wrong). Anyway, I invested some time, but quickly decided that instead of debugging the many errors I encountered, I should better switch to Xerial's implementation which required only a few bug fixes. It's a long time ago and I don't remember in detail what was wrong. The easiest way to find it out would probably be to
If an official JDBC compatibility test does not exist or cannot be acquired easily, I'd recommend to use DataNucleus with the JDO compatibility test. And this should actually become a part of SQLDroid's test suite to be automatically run during builds. |
|
Fixes #75 |
|
@jonschmidt |
|
confession: I didn't actually test it, I just looked at the code and noticed you are actually using the column filter that is not being used in the released version (the cause of the issue I reported). I would still have to confirm that it is being used properly, but from the looks of it it is. Either way, would love to see some movement on this. |
|
It's really a shame that you still did not merge my pull-request! Without any reasons. |
|
I'll take a look now. |
|
@nlmarco There are now some conflicts with master. Can you rebase the PR? I'll merge when rebased. |
|
Hi @nlmarco ! I'd like to merge this PR. Can you rebase it against master? |
I replaced basically the entire class SQLDroidDatabaseMetaData with its corresponding class found in org.sqlite.MetaData (downloaded from https://bitbucket.org/xerial/sqlite-jdbc/downloads - file sqlite-jdbc-3.7.15-m1-sources). The source project (on bitbucket.org) is licenced under the Apache Licence, hence this should be no problem.
This time, I replaced more than a few methods - I replaced the entire class and re-added a few missing methods (=> newer JDBC version).