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

return null when timestamp value is null instead of epoch default ts #151

Conversation

ivan-parada
Copy link
Contributor

@ivan-parada ivan-parada commented Jun 14, 2023

Right now ArrowResult returns default epoch timestamp 1970-01-01T00:00:00.000Z when a data type timestamp returns a null value

return new Date(value);

expected behavior is to return the null value

@dmexs
Copy link

dmexs commented Jul 15, 2023

Thanks for this, I've encountered this issue as well.

I've also noticed that nullable decimals columns are are converted to 0 instead of null.

@kravets-levko any chance this could be merged? I can try and help with a fix for nullable decimals if helpful.

@kravets-levko
Copy link
Contributor

Hi @ivan-parada! Sorry for so delayed reply, it was a tough week for me. First of all, thank you for reporting this issue and also submitting the fix 🎉 While you're on this, and considering a comment by @dmexs, I'd like to suggest you to improve the fix further. I checked the function you fixed and realized that the problem is more generic, so I'd suggest to change it like this:

diff --git a/lib/result/ArrowResult.ts b/lib/result/ArrowResult.ts
index 5bf59ec..1f49722 100644
--- a/lib/result/ArrowResult.ts
+++ b/lib/result/ArrowResult.ts
@@ -68,6 +68,10 @@ export default class ArrowResult implements IOperationResult {
   }
 
   private convertArrowTypes(value: any, valueType: DataType | undefined, fields: Array<ArrowSchemaField> = []): any {
+    if (value === null) {
+      return value;
+    }
+
     const fieldsMap: Record<string, ArrowSchemaField> = {};
     for (const field of fields) {
       fieldsMap[field.name] = field;

This should fix the issue with all data types, including date/time and decimals. Please LMK what do you think. And thank you once more for all your effort!

@dmexs
Copy link

dmexs commented Jul 17, 2023

Thanks @kravets-levko. What you're suggesting makes sense to me, but I'll let @ivan-parada weigh in too.

@dmexs
Copy link

dmexs commented Jul 24, 2023

No word from @ivan-parada but this seems like clear fix for a simple bug. Is this something i can help move forward @kravets-levko?

@kravets-levko kravets-levko merged commit 7801813 into databricks:main Jul 26, 2023
1 of 3 checks passed
nithinkdb pushed a commit to nithinkdb/databricks-sql-nodejs that referenced this pull request Aug 21, 2023
…atabricks#151)

* return null when timestamp value is null instead of epoch default ts

* Preserve null values when parsing arrow data

Signed-off-by: Levko Kravets <[email protected]>

---------

Signed-off-by: Levko Kravets <[email protected]>

Co-authored-by: Levko Kravets <[email protected]>
Signed-off-by: nithinkdb <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants