Add vroom category functions with timestamps

[krashish8]

Changes proposed in this pull request:

• Change original function name from vroom -> vroomPlain
  (denoting that the functions have plain integer types, not the TIMESTAMP/INTERVAL types)
  • sql
  • pgtap
  • doc
  • docquery
• Create functions with timestamps
  • sql
  • code (src, include)
  • pgtap
  • doc
  • docquery
• Documentation generated on my fork: https://krashish8.github.io/vrprouting/dev/en/index.html

@pgRouting/admins

[krashish8]

@cvvergara Needed some suggestions from you:

• Will it be good to create separate documentation for the new functions, or shall I just mention in the old documentation that the types of the new functions are different (TIMESTAMP / INTERVAL)?
• Creating doc / docqueries for new functions will also require creating something like vroomdata_new.sql?
• Shall I create "all" the pgtap tests? (The new functions are almost similar to old, and certain pgtap tests like inner_query consume a lot of time -> 564 plan)

Btw, this is still a draft, will make these changes before it is ready for review.

[krashish8]

Btw, is this a correct Codacy Code Analysis? If not, then shall we suppress this warning?

[cvvergara]

For pgRouting I have this legwork for Open Source Day / FOSS4G code sprint that will fix that error of the \echo
cvvergara/pgrouting#201
Basically has the detailed instructions on how to fix it.
But you can do it in one go:

perl -pi -e 's/\\echo (.*)$/\/\* $1 \*\//' docqueries/*/*.sql
tools/testers/doc_queries_generator.pl -doc

You might want to use this script
https://github.com/cvvergara/pgrouting/blob/links-to-old-pages-update/tools/developer/commitByDirectory.sh
(copy that code and add it to the tools/developer of vrpRouting)

bash tools/developer/commitByDirectory.sh docqueries "Fixing codacy errors"

The ones that can not be fixed are the \i filename.sql

[dkastl]

Trying to understand the meaning of Plain ... maybe the term is a bit misleading?

[krashish8]

This was Vicky's suggestion: plain denotes that the functions have "plain integer" types. The functions that have TIMESTAMP/INTERVAL types are suffixed without plain.

For the other functions already present such as pickDeliver, we used raw for denoting the functions with plain integers. raw looked misleading, so we decided to change it to plain. This is not changed yet, there's an issue regarding this: #8

Any other naming suggestions apart from raw or plain?

[dkastl]

OK, so then the timestamp is without suffix, and the one with integers is called PLAIN. That's fine.
I agree that RAW isn't really understandable either ;-)

I think consistency is important though. So whatever we find, a good naming should be consistent over all the functions.

Just some brainstorming for other ideas:

• Core
• Integer or int
• Basic

And what about "function overloading" and using the same function name? Or is that a bad thing? Then the function with the matching arguments/types would be selected.

[krashish8]

Yes, the timestamp is without the suffix (because that's what most of the vrprouting users will prefer to use), and the one with integers is plain.

The other naming ideas also look good, @cvvergara looking for your suggestions here. :-)

Function overloading is also a good idea, and I think that would be more convenient for the end-user. However, a "direct" function overloading won't work here, because, for the end PostgreSQL function, all the types are "same". E.g.: Both the functions vrp_vroom and vrp_vroomPlain have all the 8 arguments as TEXT. They only differ in the types of their inner queries, which isn't visible to the end PostgreSQL function.

However, as a workaround, we can implement the function overloading internally. I think in the PostgreSQL language, that should not be called "function overloading", because all the types and no. of arguments are the same. We can call it something like "overloading of the inner query types". When the inner queries are getting processed, we will need to check the types and adjust according to them.

[krashish8]

The idea looks good to me, and I think it will be more convenient for the end-user. We have something similar in the existing functions, such as ANY-INTEGER or ANY-NUMERICAL. E.g.: ANY-INTEGER denotes SMALLINT, INTEGER, or BIGINT.

We can call it... maybe, ANY-TIMESTAMP (INTEGER or TIMESTAMP) and ANY-INTERVAL (INTEGER or INTERVAL). Whichever type the internal column is, we can process it according to that. For getting the type of the result columns, we can add an extra argument to the function, , plain BOOLEAN DEFAULT FALSE. When plain is TRUE, give the final result as plain integers, when it is FALSE, give the result as TIMESTAMP/INTERVAL.

Not sure if this is considered a bad thing according to design... Maybe @cvvergara can guide us here.

[dkastl]

That's a good and valid point! Of course "automagical" is nice for the users. I would also expect that the "timestamp" usage is the common one.

What makes me a bit hesitant to create new functions, that basically do the same, is the increase of (eventually redundant) documentation, etc. And we need to think about the naming of the suffix.

Another option would be to have another optional parameter, which would be something like the "directed" or "with reverse cost" flag in pgRouting. If the parameter is not set, then the function assumes "timestamp".

What do you think about this?
Then we don't need to think about automatic detection of data types.

[krashish8]

Yes, even I am hesitant to create new functions. It looks redundant... even I mentioned that in my "first" comment of this PR. The functions work the same, so generally, there should be only a "single" page dedicated for them in the documentation.

So, if we want to have a single function (instead of two), we will need to add an optional argument, to get the final type of result columns. For the inner queries, we can make it "automagical" for the user, or determine the type based on the optional argument. I think this looks good, and we can go ahead... the automagical one would be more convenient for the user.

[dkastl]

I like ANY-TIME ;-)

But the optional parameter looks good. The position doesn't really matter if we support named parameters. I think having an optional parameter, is a good idea even if we later try to identify the data type internally, because we can add this parameter to the documentation and make the user aware of this option. Checking the data type internally (and validate it) will also allow to return more meaningful error messages, if the input data is not right.

[krashish8]

Yes, ANY-TIME looks good! Then, we will need to adjust the types for TIMESTAMP/INTERVAL internally. Semantically, both are not the same, INTERVAL is for columns such as "service time" (time duration), and TIMESTAMP is for columns such as time window open time (exact time).

I will add the optional parameter, it is definitely better. Also, even now, we check & validate the data type internally. For ANY-TIME, we will need to check for multiple types, but it would be more-or-less similar and not much harder to do.

[cvvergara]

TIMESTAMP on postgres is a int64
INTERVAL on postgres is a structure
The idea looks fine: Internally figure out the type and act accordingly.

[cvvergara]

I dont know about the users, things can get complicated if they mix up types
like timestamp on tw open and int on tw close.

[cvvergara]

I think for the current state of things, this PR is good to go.