Closed GoogleCodeExporter closed 9 years ago
GoogleCodeExporter
commented
9 years ago Ooops. I added this change:
* unspecifed length strings are TEXT, not VARCHAR (I found unspecified length
VARCHAR was not portable - at least to MySQL)
without re-running the unit tests. They blew up on (at least) the min()
function because of illegal cast from JdbcClob to String. This is fixed in my
revised patch.Original comment by James.Mo...@gmail.com on 16 Feb 2011 at 12:51
Attachments:
GoogleCodeExporter
commented
9 years ago Fixed some typos and incorrect documentation in JavaDoc comments.Original comment by James.Mo...@gmail.com on 16 Feb 2011 at 1:34
Attachments:
GoogleCodeExporter
commented
9 years ago Created patch 3 without documentation changes. Grr...Original comment by James.Mo...@gmail.com on 16 Feb 2011 at 1:40
Attachments:
GoogleCodeExporter
commented
9 years ago Hi,
Thanks a lot for the patch! It's quite large, I guess it will take a while
until it's merged. It seems it would be best if I give you commit rights if you
plan to continue to work on JaQu. However, first we need to go over the details
of the patch.
1) You copied a few classes from org.h2.util to src/tools/org/h2/jaqu/util. Is
this required? Is the idea to make JaQu independent of H2? If yes, that's
theoretically OK, but it would duplicate source code, which I'm against because
it's a pain to maintain. Specially, if I have to maintain it. I would rather
have a strong dependency from JaQu to H2. At the moment I'm not willing to
create yet another project for the common code. There is a solution however:
change the build so that it includes only the required classes when building
the JaQu jar file (so the JaQu jar file doesn't contain the complete H2 jar
file).
2) EventLogger / Slf4jLogger: I would like to avoid / simplify this. Could the
H2 logging mechanism be re-used?
3) JQColumn / JQIndex / JQTable: I would like to rename them to Column / Index
/ Table. This may require other classes to be renamed (Table), but maybe not.
4) Annotations and indexes: is it possible to declare multiple indexes per
table? If yes how? How to declare indexes with multiple columns? Is there a way
to make it compile time save (detect typos in column names)? The same for
primary keys.
5) Javadocs: missing for TableInspector. What is this class for?
6) General about patches: did you read
http://h2database.com/html/build.html#providing_patches ?
Original comment by thomas.t...@gmail.com on 16 Feb 2011 at 8:10
GoogleCodeExporter
commented
9 years ago Some more remarks:
* schema support
Sound good!
* boolean, byte, short, float support
Is it boolean or java.lang.Boolean etc.?
* unspecifed length strings are TEXT, not VARCHAR (I found unspecified length
VARCHAR was not portable - at least to MySQL)
It sounds like this is a performance problem, even for MySQL. I believe TEXT is
CLOB in MySQL, which is probably stored externally and therefore much slower
than VARCHAR.
* models may be defined by Annotations as alternative to Table interface
It's fine if it's optional.
* automated model generation from traversing database
OK if it's completely optional (even thought I personally don't think it's a
good idea to generate source code).
* model validation by traversing database
This I don't understand. What do you validate?
* annotated tables bypass the static synchronized Define architecture
Do you think it's a problem because it's synchronized? I don't think so, it's
just once per class.
* table annotation to bypass createIfRequired step
OK.
* table annotation to inherit column annotations from super class
I didn't really think about super classes so far.
* table annotation to control strict typing (i.e. no mapping to VARCHAR for
unsupported types)
OK.
* table annotation to create memory table (H2 only, confirmed by connection
class check)
Maybe the 'persistence' configuration should be on another level, or on another
level in addition to an annotation. For example: when running tests, everything
is in-memory. Otherwise use persistent tables.
* annotated column fields may be of any scope ( field.setAccessible(true) )
OK. setAccessible(true) is already used for constructors, that's fine.
* column annotation for auto increment
OK.
* column annotation to automatically trim strings to maxLength on insert and
update
OK, event thought I wonder if this is flexible enough... well let's see. Maybe
we will need some kind of validation framework later on.
* column annotation for default values
Sounds like a good idea.
* Db(Connection conn) constructor for working with pools or to slipstream into
legacy designs
I forgot that.
* long Db.insertAndGetKey(o) method (generated keys)
* List<Long> Db.insertAndGetKeys(List<o>) method (generated keys)
* Db.delete(o) method
* Db.deleteAll(List<o>) method
* set(field).to(value) syntax implemented
* increment(field).by(step) syntax implemented
* QueryWhere.update() to execute set(field) and increment(field)
* limit(long) syntax implemented
* offset(long) syntax implemented
* implemented missing CREATE INDEX functionality with support for NORMAL,
UNIQUE, HASH, and UNIQUE HASH indexes
* SQLDialect interface with default implementation for handling variations in
SQL syntax (currently tableName, LIMIT, OFFSET, and CREATE INDEX)
* database annotation to specify database model version
* table annotation to specify table model version
* automatic database model version checking with delegation to user-provided
DbUpgrader implementation for out-of-date databases.
* automatic table model version checking with delegation to user-provided
DbUpgrader implementation for out-of-date tables.
Sounds good!
* implemented rudimentary event logger based on java.util.Logging which can
optionally interface to SLF4J (mostly for debugging generated sql statements)
I would like to avoid this if possible (let's see).
* ensure Statements are also closed, not just ResultSets
OK.
* changed volatile long counter in Utils to AtomicLong. long counter++ makes
me uneasy for atomicity. Maybe its atomic on 64-bit system. Not sure. Got a
good resource/explanation for why it would be atomic?
AtomicLong is better, I agree.
* removed dependencies on non-JaQu package code (jaqu.jar would only be usable
when paired with h2.jar, fine for H2, not so fine for other DBs).
We will find a solution here.
* added new unit test models and unit tests to cover all(I think) changes
That's great! I would like to have good code coverage (it doesn't need to be as
high as 90%).Original comment by thomas.t...@gmail.com on 16 Feb 2011 at 8:26
GoogleCodeExporter
commented
9 years ago 1. Copying sources. I considered modifying the build, but the trouble is those
utility classes have dependencies on other classes which could trigger class
verify errors in their absence.
The copied bits seem likely not to change as they are well-used utility
functions. I did not copy whole classes, just referenced functions.
Independence is a goal just because otherwise the 100K JaQu lib has a 1.2M
dependency. The only function that is Lesser than the original is the
loadClass function. The H2 version does some additional things which I didn't
really understand. :)
2. Trace system. Possibly, see #1. My first concern was being able to monitor
the generated statements - since JaQu is a statement generator. Secondly (in
my workspace) I have added counters to track # of insert, update, create, etc
statements generated. I'm finding that useful in benchmarking straight JDBC vs
JaQu vs H2 vs MySQL. Not sure that could be achieved with Trace. I didn't
really want a framework, just a simple say to capture statements & counts.
Slf4jLogger can be removed. I'm not using it, just wrote it for completeness
and since you offered SLF4J inside H2.
3. Renaming is fine.
If I were king, I'd drop the Table interface and Define class altogether. Yes,
its threadsafe, but if you're using it in a server with many concurrent users
its unlikely that its called just once so I think it becomes an unnecessary
bottleneck.
In the preliminary port of one of my projects, my concurrency unit test would
call Define.define() about 300,000 times in 24 seconds, if I was using the
Table interface. Granted my preliminary port is stupid and not optimized, but
I know the end result will be mapping objects alot more than once.
Did you mean for Db to be a singleton? Or once per connection? My plan for my
project was once per pool retrieval, not per-user or per-db. Basically
per-request, but that request may result in a cascade of db.from() calls
depending on that data the user is requesting. So I will be mapping objects
and fields alot.
4. JQIndex currently supports multiple indexes (and index types) per table.
Same with multiple column indexes and primary keys. It is not possible to make
this part compile-time safe as you are naming columns, not variables - this is
one of the differences between my annotation approach and your interface
approach. But I did write a validator to ensure that the column names are
correct (see #5).
@JQIndex(standard="id date invoiceid", hash="invoiceid")
@JQIndex(standard={"id date invoiceid", "a b c" }, hash="invoiceid")
@JQTable(primaryKey="id")
@JQTable(primaryKey="id date")
5. TableInspector works with DbInspector and generates annotated model classes
from a database.
It also can validate a model class (annotated or interface or both) against a
database to ensure schema matches, table names match, columns match, types
match (e.g. TIMESTAMP != java.lang.Short), default values are valid
(incomplete), etc.
It also performs index validation (incomplete).
Validation returns a list of validation remarks of varying levels; ERROR, WARN,
CONSIDER which the user may choose to ignore or implement or whatever.
6. I did review the "providing patches doc" before submitting. I admit to not
using the code coverage tool. I think I have your Eclipse settings, but it
didn't seem to format the same so I mostly hand-broke lines and what-not.
Nothing that Eclipse can't fix, though.
Object types Boolean, Byte, Short, and Float were added, not the primitives.
The trim strings parameter is per-column, not per table and not for the whole
db. I think that if the user needs finer granularity than per-column, then
they should probably implement string handling themselves.Original comment by James.Mo...@gmail.com on 16 Feb 2011 at 9:10
GoogleCodeExporter
commented
9 years ago I tried to use your build coverage. This doesn't seem to be working for me.
Out-of-memory test kills the whole process prematurely. If I comment that out,
it seems to chug along but takes a very long time to chug through all the
iterations.... and even when completing the iterations, JaQu code does not seem
to be "covered" at all.
I took a stab at implementing a jaquCoverage() target, but failed to get that
working.
How do you want to proceed with patch?Original comment by James.Mo...@gmail.com on 18 Feb 2011 at 3:55
GoogleCodeExporter
commented
9 years ago Don't worry too much about the code coverage, I will add that to my list. You
are right, it makes sense to have a special code coverage test for JaQu,
because the other tests take about 3 hours.
1. Copying sources. I will ensure the utility classes that are used within JaQu
have no dependencies on the database. Maybe I will have to change some classes.
Copying a few methods is OK, but copying whole classes is just not
maintainable.
2. Trace system. I'm not sure yet what the best solution would be.
3. Table interface and Define class: if it's called more than once per VM/class
loader then I will fix that.
4., 5., 6. OK.
How should we continue? If you have changed anything yet, then could you submit
your new patch, so I will review it? If not, I will review the last one. I will
then apply it on my side and discuss with you what I would like to change in
detail, and I will then attach my patch here and give you commit rights. It's a
bit complicated, I know, but usually people send smaller patches :-) I hope
this is OK for you.
Original comment by thomas.t...@gmail.com on 18 Feb 2011 at 4:59
GoogleCodeExporter
commented
9 years ago Don't worry too much about the code coverage, I will add that to my list. You
are right, it makes sense to have a special code coverage test for JaQu,
because the other tests take about 3 hours.
1. Copying sources. I will ensure the utility classes that are used within JaQu
have no dependencies on the database. Maybe I will have to change some classes.
Copying a few methods is OK, but copying whole classes is just not
maintainable.
2. Trace system. I'm not sure yet what the best solution would be.
3. Table interface and Define class: if it's called more than once per VM/class
loader then I will fix that.
4., 5., 6. OK.
How should we continue? If you have changed anything yet, then could you submit
your new patch, so I will review it? If not, I will review the last one. I will
then apply it on my side and discuss with you what I would like to change in
detail, and I will then attach my patch here and give you commit rights. It's a
bit complicated, I know, but usually people send smaller patches :-) I hope
this is OK for you.
Original comment by thomas.t...@gmail.com on 18 Feb 2011 at 5:23
GoogleCodeExporter
commented
9 years ago Updated Patch.
* Eliminated previous logger classes.
* Added StatementLogger which is a static wrapper for a System.out (or other
PrintWriter) and does some stats for me. Writing to System.out is optional and
disbaled by default. That is really all I need.
* Added a bunch of JavaDoc on classes.
* Implemented most defaultValue validation. H2 will have pretty good
validation, I think. MySQL not as much since the defaultValue values (yuck)
are not single-quoted for literals as they are in H2.Original comment by James.Mo...@gmail.com on 18 Feb 2011 at 10:36
Attachments:
GoogleCodeExporter
commented
9 years ago The features of this issue were integrated in early Spring and massaged into
proper form by Thomas in the weeks after submission. Closing issue.Original comment by James.Mo...@gmail.com on 30 Jun 2011 at 11:17
Original issue reported on code.google.com by
James.Mo...@gmail.comon 15 Feb 2011 at 10:53Attachments: