Skip to content

Again trying to complete SQLDroidDatabaseMetaData#39

Open
nlmarco wants to merge 3 commits intoSQLDroid:masterfrom
cloudstore:master
Open

Again trying to complete SQLDroidDatabaseMetaData#39
nlmarco wants to merge 3 commits intoSQLDroid:masterfrom
cloudstore:master

Conversation

@nlmarco
Copy link
Copy Markdown

@nlmarco nlmarco commented Apr 23, 2014

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).

@ergouser
Copy link
Copy Markdown
Contributor

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
from xerial.

There is some, fairly minimal, test code that (IIRC) tests some of the
database meta data. I would expect them to pass, but it's probably
worth running them.

Jim

On 04/23/2014 07:09 AM, nlmarco wrote:

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).


    You can merge this Pull Request by running

git pull https://github.com/cloudstore/SQLDroid master

Or view, comment on, or merge it at:

#39

    Commit Summary


Reply to this email directly or view it on GitHub
#39.

Jim Redman
(505) 662 5156 x85
http://www.ergotech.com

@nlmarco
Copy link
Copy Markdown
Author

nlmarco commented Sep 4, 2014

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 :-)

@donv
Copy link
Copy Markdown
Member

donv commented Aug 15, 2016

Hi all!

This PR is pretty old now. Is it still viable?

@donv
Copy link
Copy Markdown
Member

donv commented Aug 19, 2016

Hi @nlmarco !

Do you still want to work on this PR? If not, we'll close it. We still want SQLDroidDatabaseMetaData implemented and complete.

@donv donv added this to the 1.1.0 milestone Aug 19, 2016
@donv donv added the Internal label Aug 19, 2016
@nlmarco
Copy link
Copy Markdown
Author

nlmarco commented Aug 20, 2016

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:

  1. Is my understanding correct that our pull-request was already accepted and thus merged into the official codebase? If not, why does it state "Merged" above?

  2. Do you want me to do more work on some classes related to the work we already did?

  3. If so, which classes exactly and why? "Why" means, what exactly do you want to achieve? Compatibility with a certain ORM or other system using the JDBC API?

Best regards, Marco :-)

@nlmarco
Copy link
Copy Markdown
Author

nlmarco commented Aug 20, 2016

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
@nlmarco
Copy link
Copy Markdown
Author

nlmarco commented Aug 20, 2016

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.

@jhannes
Copy link
Copy Markdown
Contributor

jhannes commented Aug 20, 2016

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.

@nlmarco
Copy link
Copy Markdown
Author

nlmarco commented Aug 21, 2016

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:

  1. Xerial's version was far more complete than SQLDroid's version.

  2. Xerial is the author of sqlite. It is therefore preferrable to take as much from their code as possible. I don't understand, why SQLDroid didn't do this in the first place, anyway.

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

  1. Use an official JDBC compatibility test suite. I don't know, if this exists.

  2. Use one (or better multiple) ORMs like DataNucleus, EclipseLink and the like and make it dynamically create the schema. This is what I did - as already explained, before.

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.

@jonschmidt
Copy link
Copy Markdown

Fixes #75

@nlmarco
Copy link
Copy Markdown
Author

nlmarco commented Sep 13, 2016

@jonschmidt
Thanks a lot for testing! Seems, my version works better than the current official version - which I btw. expected ;-) Did you encounter any bugs?

@jonschmidt
Copy link
Copy Markdown

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.

@nlmarco
Copy link
Copy Markdown
Author

nlmarco commented Sep 15, 2020

It's really a shame that you still did not merge my pull-request! Without any reasons.

@donv
Copy link
Copy Markdown
Member

donv commented Sep 15, 2020

I'll take a look now.

@donv
Copy link
Copy Markdown
Member

donv commented Sep 15, 2020

@nlmarco There are now some conflicts with master. Can you rebase the PR? I'll merge when rebased.

@donv
Copy link
Copy Markdown
Member

donv commented Oct 13, 2020

Hi @nlmarco !

I'd like to merge this PR. Can you rebase it against master?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants