add toCalendar method #7081 , and fix astar graph function #7103 some bugs

[saeedtabrizi]

• fix bug report for astar graphdb in A* SQL function returns paths of increasing length for consecutive, identical queries #7103 issue
  -fix null checking in astar function .
• add toCalendar method as we discussed on Better support for time expression Better support for time expression #7081.
• add some test cases for toCalendar Method . (need to be documented) .

[luigidellaquila]

Hi @saeedtabrizi

Thank you very much, I'm checking the PR about the calendar.
About the A* function I'm afraid your fix does not work in multi-thread... anyway I found the root cause and fixed it, the problem was how the function was registered in OFunctionFactory:

register(OSQLFunctionAstar.NAME, new OSQLFunctionAstar());

This way the server has a single instance of the function. I changed it with

register(OSQLFunctionAstar.NAME, OSQLFunctionAstar.class);

so now the server returns a new instance at every invocation.

Could you please re-submit the PR removing the changes to A*

Thanks

Luigi

[saeedtabrizi]

Hi @luigidellaquila .
Thanks to solve the root of problem for multiple instance in A* function , but this PR have the some null value checking that would be in the A* function . i detect this problem when i review the A* code and i write a test method to cover it .
So this PR not only related to A* multiple function and have the changes for the A* null checking too .

[luigidellaquila]

Got it, thank you @saeedtabrizi
I'll merge it with the rest then

Thanks!

Luigi

[saeedtabrizi]

@luigidellaquila , @lvca , Is there any chance to have this merge in 2.2.17 release and change the milestone from 3 to 2.2.17 ?

[lvca]

Hi @luigidellaquila what's the status of this?

[saeedtabrizi]

Hi @lvca , @luigidellaquila
Is there any chance to have this PR in ODB 3.0 :)

[saeedtabrizi]

@lvca , @luigidellaquila I guess we have not any chance to have this pull request in ODB v3.0 .

[luigidellaquila]

Hi @saeedtabrizi

checking it now. Just a couple of doubts:

• the A* function is already fixed, could you please remove that part from the PR?
• I see you added a new library (ICU4J) that has a different license from Apache2. I had a quick look at that, but I'd like @lvca to double-check and make sure that it is OK
• as far as I can see the toCalendar() method is not registered in ODefaultSQLMethodFactory so it won't work from SQL. I think it's worth registering it and adding some SQL tests, WDYT?
• I guess the main purpose of this PR is to provide an API to modify dates from SQL, but to do this we also will have expose some additional methods as OSQLMetods, like add() subtract() and so on, did I get it right?

Thanks

Luigi

[saeedtabrizi]

Hi @luigidellaquila ,
First of all i'm observing ODB 3.0 changes . that must be great and i appreciate to orientdb team .

• For A* function i wrote some test and fixing the null value checking . but your fixture completely solve the problem and i agree to remove from PR .

• For the licencing and permission of ICU4J we have already discussed in Better support for time expression #7081 . some major platform , library or applications like nodejs using the ICU .

• For the toCalendar() registering in the ODefaultSQLMethodFactory , i have not any idea and i'm agree for any changes that help us to using calendar functionalities in ODB .

As a summary , this PR goal is adding the calendar calculation and time methods to ODB 3.0 via ICU4J . i write a wrapper class and methods too .

Thanks
Saeed Tabrizi

[saeedtabrizi]

Hi @luigidellaquila
Is there any chance to have merged this PR in ODB 3.0 M2 ?

Thanks

[luigidellaquila]

Hi @saeedtabrizi

Sure, my intention is to merge it, but to do it you should remove the changes to AStar, as it's already fixed in the main branch.
About the registration of toCalendar() in the function factory, there is no need to do it in the PR, I can do it separately

Thanks

Luigi

[saeedtabrizi]

Hi @luigidellaquila
Ok i remove the Astar changes and push it again .
Thanks

[saeedtabrizi]

Hi @luigidellaquila ,
As i review my code i have the following changes in Astar function that must be important to include for Astar safe running .

 open.clear();
   closedSet.clear();
   route.clear();
   currentDepth=0;

As i see the history of OGraphFunctionFactory file i have not any modification about it and you can merge it without any problem .

So please merge this PR and put it to 3.0 milestone .
I just started to work about anothe graph sql functions that we discussed in #7132 (comment) .

Thanks .
Saeed Tabrizi

[luigidellaquila]

Hi @saeedtabrizi

I did a deep review of this PR and I still have a couple of doubts.

I guess the main purpose of this PR is to have a way to manipulate dates in SQL, if I'm wrong please correct me, but I cannot find any other rationale
The problem is that as it is it, won't work in SQL for at least three reasons:

1. the function toCalendar() is not registered as a SQL function, so it just doesn't work from SQL. As I wrote before, I can easily add it, so it's not a problem

2. the function returns an OCalendar object, that is not a standard OrientDB type, so the binary protocol doesn't know how to serialize it.

To make it clear, the following query will fail in remote

SELECT toCalendar( "20151004T00:00:00") as theCal

because the protocol doesn't know how to serialize theCal

3. I guess the main goal of OCalendar is to manipulate dates in SQL (again, correct me if I'm wrong), so it has some Java methods like add(), subtract(), getMonth() and so on. These methods are not exposed as SQL methods, so they won't be accessible in SQL queries, eg. the following

SELECT toCalendar( "20151004T00:00:00").getMonth()

will fail because getMonth() is not a registered SQL method.
To fix this, we should write a lot of separate OSQLMethod implementations, one for each method on OCalendar.

Please help me to understand if I'm missing something, my feeling is that we still have a lot of things to do to merge this PR...

Thanks

Luigi

[saeedtabrizi]

Hi @luigidellaquila
The goal of using calendar base date & time calculation is achieving more relaxation in globalized application queries . so i recommended to have an extension method for Date data type exactly so its not a function .
The true syntax for working with calendar must be :
Select Birthday , Birthday.toCalendar("persian").getMonth() as PBM from User .

Result :

    Birthday  |  PBM
--------------|-------
    1982-3-22 |  1       // The thirth month of gregorian calendar equals to first month of persian calendar . 
------------------------

That Birthday is a DateTime data type and toCalendar method returns an instance of special calendar that implemented some methods . it just very similar to format method for DateTime data type in orientdb . in the other word my template for DateTime method extending is the date format method implementation in the ODB .
I have defined multiple sample for added methods and functions for implementing calendar and time span calculation in #7081 issue and we already discussed about syntax of it.

I hope to transfer my idea for calendar implementation clearly .

Thanks
Saeed Tabrizi

[lvca]

Very cool and useful contribution, +1. @saeedtabrizi please look at the output from Codacy for static warnings and errors: https://www.codacy.com/app/OrientDB/orientdb/pullRequest?prid=495581.

[saeedtabrizi]

Many Thanks @lvca . I just lint the code and try to cleanup errors and warning :)

[lvca]

I was reading Luigi's comments and he's right. You should register a lot of new methods in order to be used in SQL. At that point, the question is: why using getCalendar() if you could apply such methods directly to the date types? Like select birthday.getMonth(). getCalendar() could be useful only if you want to change calendar at this point. Am I missing anything?

[saeedtabrizi]

@lvca the short answer is yes . i use toCalendar method for instantiate a calendar that using in date calculation operations .
So i have some mistakes to implementing the date and calendar calculation related methods in orientdb .
In my case the OCalendar class have an wrapper role exactly and when calling the toCalendar method in DateTime instances an OCalendar class instantiated and all of OCalendar method now can call by the user .

It is so clear and simple . but as i understand from you and dear @luigidellaquila comments, i should implement all methods of OCalendar class as a orientdb sql method too . is it true ?
for example if i have support the following syntax :
select * from Log where logDate >= logDate.toCalendar('persian').add('YEAR',5).toDate() ;

Which .toCalendar('persian') returns an OCalendar instance and .add('YEAR',5) accumulates 5 years adding to OCalendar instance and returns OCalendar and finally by calling .toDate() method of OCalendar it just returns a standard Date datatype , I should implement an add() method and a toCalendar() and a toDate() method in orientdb sql methods.

For the your question about using the toCalendar method , you are right . we can have some methods directly used by Date datatype like the select birthday.getMonth('persian') that is a shortcut for select birthday.toCalendar('persian').getMonth() syntax .
Please correct me if i miss understood .

Thanks Saeed .

[saeedtabrizi]

Hi @lvca , @luigidellaquila
Is there any alternative to support calendar and time utils in ODB ?

[luigidellaquila]

Hi @saeedtabrizi

I'm afraid for now there are no further alternatives, we are more or less at the same point of when this issue was reported

Thanks

Luigi