strptime function definition violates spec

[vbarua]

Context

While upgrading substrait-java to the latest version of the spec, tests failed due to multiple scalar functions being added w/ the same type signature.

As defined, strptime

substrait/extensions/functions_datetime.yaml

Lines 405 to 442 in e8ced2a

	name: "strptime"
	description: >-
	Parse string into timestamp/date/time using provided format,
	see https://man7.org/linux/man-pages/man3/strptime.3.html for reference.
	If timezone is present in timestamp and provided as parameter an error is thrown.
	Timezone strings must be as defined by IANA timezone database (https://www.iana.org/time-zones).
	Examples: "Pacific/Marquesas", "Etc/GMT+1".
	If timezone is supplied as parameter and present in the parsed string the parsed timezone is used.
	If parameter supplied timezone is invalid an error is thrown.
	impls:
	- args:
	- name: timestamp_string
	value: string
	- name: format
	value: string
	- name: timezone
	description: Timezone string from IANA tzdb.
	value: string
	return: timestamp_tz
	- args:
	- name: timestamp_string
	value: string
	- name: format
	value: string
	return: timestamp_tz
	- args:
	- name: date_string
	value: string
	- name: format
	value: string
	return: date
	- args:
	- name: time_string
	value: string
	- name: format
	value: string
	return: time

has the following signatures

* strptime:str_str_str
* strptime:str_str
* strptime:str_str
* strptime:str_str

Problem

The addition of the the strptime function in #272 violates the spec.

Specifically

It is required that two function implementations with the same simple name must resolve to different compound names using types. If two function implementations in a YAML file resolve to the same compound name, the YAML file is invalid and behavior is undefined.

https://substrait.io/extensions/#function-signature-compound-names

Changes were introduced by #272

[rok]

Oh I missed this. How about: strptime -> strptime_timestamp, strptime_date, strptime_time?

[richtia]

Oh I missed this. How about: strptime -> strptime_timestamp, strptime_date, strptime_time?

This is what I was thinking as well...separating the function into 3 separate ones. I'm curious how producers/consumers will handle this with regards to mapping though.

[rok]

I'm curious how producers/consumers will handle this with regards to mapping though.

What would you expect to be an issue there?

[richtia]

What would you expect to be an issue there?

Not necessarily expecting any issues. Just wondering how it would work since I've typically seen a single strptime function. I guess all 3 from substrait would just map the 1? Any thoughts @westonpace?

[richtia]

@rok I went ahead and submitted a PR for this since I have a few other PRs waiting for the fix.

[rok]

Thanks @richtia !

[richtia]

Thanks @richtia !

Np! Do you mind giving it a quick look? #493

[richtia]

Should be resolved by #493 now