my2iu / Jinq

LINQ-style queries for Java 8
Other
660 stars 71 forks source link

JPQL date methods #30

Open aVolpe opened 8 years ago

aVolpe commented 8 years ago

In JPQL we can use date functions (see this doc).

In Jinq, we can't use this because new Date() is not recognized, and in the JPQL class there is not functions to handle this case.

What are the steps required to implement something like this? Or this is not in the scope of Jinq?

Thanks for the wonderful library.

my2iu commented 8 years ago

I remember looking at this, but I don't fully remember my reasoning for not including it.

I think it was that JPQL supplied functions for getting the current date/time, but I couldn't find a way to actually manipulate the date/time. As a result, the functions seemed useless to me. So if you wanted to run a query to find all the sales within the last 7 days, I couldn't find a way to subtract 7 days from the current date, so I couldn't figure out how those functions would actually be used in a query.

In order to do useful queries involving the current date/time in JPQL, it seemed like you had to do all the math on the Java side anyway, making it pointless to support the CURRENT_DATE, etc. functions. Jinq does support that. Just pull the date calculation out of the query and pass it in as a query parameter.

Date sevenBefore = Date.valueOf(LocalDate.now().minusDays(7));
salesStream.where( s -> s.date().after(sevenBefore) )

or something like that.

aVolpe commented 8 years ago

@my2iu there are valid cases to user the CURRENT_DATE, etc functions.

I create a pull request, #31 with the implementation of the functions, and a Test to try it, I don't know if this is the best way to approach this with JINQ, but it's a try.

In the class SymbExToColumns exists a assert that can't be validated with the new functions, so I add a XXX and a ugly solution to this, I cannot find a way to check hierarchy with the Type class.

The new API of Java is very convenient, but this is a part of the specification and I think there are cases where the date precision of the database are needed.

One mor time, thanks for your wonderful library.

My TODO is to add the documentation to the changes I made if you like the change.

my2iu commented 8 years ago

Thanks for the code changes.

It seems that the CURRENT_DATE etc. functions are mostly used for updates or for people using proprietary variants of JPA that have been extended with date manipulation functions. These types of things aren't currently supported in Jinq. As such, I'm a little reluctant to include support for it because if people see the functions in the documentation, they might spend a long time trying to use them when they aren't actually useful. They should have instead been spending their time using dates as parameters, which is the proper JPQL way of doing things at the moment.

As such, I'm going to keep your changes around for future use, but I won't merge them in at the moment. I'm going to hold off until

  1. I get around to adding update support to Jinq, which is where having a CURRENT_DATE function would be mostly useful
  2. or when JPA adds support for the new Java 8 time stuff, and I have to do a more thorough reworking of Java 8's time support
aVolpe commented 8 years ago

If I create a pull request with the fix to your test regards the java.sql API you will accept that? These changes are not related to the current_date issue and only fix a minor bug.

As you say, the CURRENT_DATE and related are not useful for the main case of Jinq, the best use case is to create the ranges in java.

Thanks for your library.

my2iu commented 8 years ago

Thanks for finding that bug! I'll actually go fix that myself. I'll try to fix it using more Java 8 stuff because apparently all that old java.sql.Date stuff is just very ambiguous about time zones anyway so should generally be avoided.