Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

R UDF with Date type fails to convert RAPI #6674

Closed
monetdb-team opened this issue Nov 30, 2020 · 0 comments
Closed

R UDF with Date type fails to convert RAPI #6674

monetdb-team opened this issue Nov 30, 2020 · 0 comments

Comments

@monetdb-team
Copy link

@monetdb-team monetdb-team commented Nov 30, 2020

Date: 2019-01-06 14:47:26 +0100
From: Paul <>
To: MonetDB5 devs <>
Version: 11.31.11 (Aug2018-SP1)
CC: @kutsurak

Last updated: 2019-04-30 12:36:04 +0200

Comment 26779

Date: 2019-01-06 14:47:26 +0100
From: Paul <>

User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:64.0) Gecko/20100101 Firefox/64.0
Build Identifier:

When an R UDF is defined with an output using a date type field as an output argument of an embedded R UDF the field fails with a message "Failed to convert column 0"

Reproducible: Always

Steps to Reproduce:

1.Create a table with dates

CREATE TABLE testTableDates (d DATE);
INSERT INTO testTableDates(d) VALUES ('2019-01-01'),('2019-01-02'),('2019-01-03');
  1. Create a simple loopback R based UDF
CREATE FUNCTION testDateLoopback(din DATE) RETURNS TABLE (dout DATE) LANGUAGE R {
   data.frame(dout=din)
}
  1. Call loopback the UDF
    SELECT * FROM testDateLoopback( (SELECT * FROM testTableDates) )

Actual Results:

"Failed to convert column 0"

Expected Results:

'2019-01-01'
'2019-01-02'
'2019-01-03'

Dug into the code base and found that there is a TODO for this but cannot find any ticket raised for it:

bat_to_sexp: Works sufficiently due to bat type being an integer although R sees the date as an integer.

sexp_to_bat: Looks at the UDF arg type and fails to convert as no TYPE_Date defined

I have created an initial fix for this ready for review but cannot find any details about how to access the repository in order to push these changes and contribute to the project. Please advise on how to proceed.

Comment 26780

Date: 2019-01-07 17:29:09 +0100
From: @kutsurak

Hello Paul,

The official MonetDB mercurial repository is hosted at

https://dev.monetdb.org/hg/MonetDB/

You can prepare a patch (preferably with the command hg export) and add it as an attachment to this issue so that someone can review it. If you need more information let us know.

Best regards,
Panos.

Comment 26784

Date: 2019-01-09 13:28:52 +0100
From: Paul <>

Created attachment 613
initial patch for review

Attached file: b6674_Rapi (text/plain, 7317 bytes)
Description: initial patch for review

Comment 26785

Date: 2019-01-09 13:29:15 +0100
From: Paul <>

Thanks Panos,

I've attached an initial patch for review. I've fixed the date output and also the representation within R.

I was tempted to refactor some code in mtime as I noticed it defines a date_isnil but other types are in the format id_{type}_nil. I've created an alias define in this patch. Can we refactor mtime or is there a particular reason for this inconsistent naming?

Also, I'd like to add some tests for the changes but not sure how best to write them in the bat format.

Is there a quick way to generate the following in bat format?

//Proves class is Date rather than Numeric
CREATE FUNCTION getClassName(din DATE) RETURNS TABLE (dout TEXT) LANGUAGE R {
data.frame(dout=class(din))
}
SELECT * FROM getClassName( (SELECT * FROM T001) )

//Prove that conversion is correctly offsetting for epoch
CREATE FUNCTION passthroughConversionCheck(din DATE) RETURNS TABLE (dout DATE) LANGUAGE R {
data.frame(dout=as.Date(as.double(din), origin="1970-1-1") )
}
SELECT * FROM passthroughConversionCheck( (SELECT * FROM T001) )

Comment 26791

Date: 2019-01-14 16:00:16 +0100
From: @kutsurak

Hi Paul,

Thanks for the patch. Part of our team is in the middle of moving to a new building so this might take some time to process.

About your question with respect to mtime, revisiting this code has been one of our TODO items for some time now, but as far as I know there has been a number of related issues that we would like to visit at the same time. I will check and get back to you as soon as I can.

Thank you again,
Panos.

Comment 26792

Date: 2019-01-14 16:15:53 +0100
From: Paul <>

Thanks for the update Panos, good luck with the move.

Useful to know that about mtime, I figured as much given the almost "here be dragons" style comments and the well documented history of time in there, fun read!

That reminds me I was doing further testing and can't quite get my head around one of the results.

A date defined as null returns as null with a direct query, but when it involves a call to an R UDF it returns as nil. I thought I had used the int null check rather than dbl null check but switching those makes no difference difference. When you have a moment I'd love to get your thoughts on where that comes from - I wasn't aware nil was a valid value to store.

Comment 26873

Date: 2019-02-04 15:37:33 +0100
From: MonetDB Mercurial Repository <>

Changeset fe2012cbd8dc made by Sjoerd Mullender sjoerd@acm.org in the MonetDB repo, refers to this bug.

For complete details, see https//devmonetdborg/hg/MonetDB?cmd=changeset;node=fe2012cbd8dc

Changeset description:

Applied patch from bug #6674.

Comment 26874

Date: 2019-02-04 17:35:49 +0100
From: @sjoerdmullender

(In reply to Paul from comment 3)

I was tempted to refactor some code in mtime as I noticed it defines a
date_isnil but other types are in the format id_{type}_nil. I've created an
alias define in this patch. Can we refactor mtime or is there a particular
reason for this inconsistent naming?

Historical reasons.
I've now changed the macros to is_date_nil etc.

Comment 26932

Date: 2019-03-19 09:29:10 +0100
From: MonetDB Mercurial Repository <>

Changeset ee38b509dbe9 made by Sjoerd Mullender sjoerd@acm.org in the MonetDB repo, refers to this bug.

For complete details, see https//devmonetdborg/hg/MonetDB?cmd=changeset;node=ee38b509dbe9

Changeset description:

Added test for bug #6674.

Comment 26933

Date: 2019-03-19 09:30:18 +0100
From: @sjoerdmullender

Patch applied, test added. Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
1 participant