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

Node Library Error: Data type is not supported TIME #3344

Closed
2 tasks done
hamilton opened this issue Apr 1, 2022 · 4 comments
Closed
2 tasks done

Node Library Error: Data type is not supported TIME #3344

hamilton opened this issue Apr 1, 2022 · 4 comments
Labels

Comments

@hamilton
Copy link
Contributor

hamilton commented Apr 1, 2022

What happens?

When I attempt to select a TIME, duckdb throws the error:

Node Library Error: Data type is not supported TIME

Probably straightforward to add; just need to add a case statement for duckdb:LogicalTypeId::TIME to this part of statement.cpp.

Any helpful hints on what the TIME value is under the hood would be much appreciated. I suspect its just microseconds. If so, I think it would probably make sense to just return an integer representing the microseconds since there isn't really anything comparable to TIME in ECMAscript.

To Reproduce

Grab the node library and write a query for SELECT current_time; and you should receive an error:

const duckdb = require('duckdb')
const db = new duckdb.Database(":memory:");
db.all('SELECT current_time', function(err, res) {
    if (err) {
      throw err;
    }
    console.log(res[0])
  });

Environment (please complete the following information):

  • OS: mac os 12.1 M1
  • DuckDB Version: 3.2 & 3.3-latest
  • DuckDB Client: Node.JS

Before Submitting

  • Have you tried this on the latest master branch?
  • Python: pip install duckdb --upgrade --pre
  • R: install.packages("https://github.com/duckdb/duckdb/releases/download/master-builds/duckdb_r_src.tar.gz", repos = NULL)
  • Other Platforms: You can find binaries here or compile from source.
  • Have you tried the steps to reproduce? Do they include all relevant data and configuration? Does the issue you report still appear there?
@hannes
Copy link
Member

hannes commented Apr 1, 2022

Indeed we should probably get all the types supported for node. What would you like to get on the node side for a TIME type? A string?

@hamilton
Copy link
Contributor Author

hamilton commented Apr 1, 2022

Good question. I think there are probably three options:

  • microseconds (number) – provide microseconds to the user and have them handle it. In terms of ergonomics, this will be foreign to most JS developers since they almost always are dealing with ms, but in practice shouldn't be a problem. For the vast majority of times, it's likely to be well within Number.MAX_SAFE_INTEGER, and if not could always be switched to a BigInt.
  • time string as "HH:MM:SS.dddddd" – let the user parse / handle this string.
  • object with { hours, minutes, seconds, fractional } – this is the approach taken by the BigQuery node API. This saves the user from having to inevitably perform the next step of parsing a string or converting microseconds.

I went ahead and asked in a Twitter poll to get a sense of what the node.js / data tools community might expect. We can take the results of that with a grain of salt of course :)

I have no preference between these three choices. I already submitted a PR a few months ago to convert INTERVAL to an object so there's some precedence for duckdb to return objects.

@github-actions
Copy link

This issue is stale because it has been open 90 days with no activity. Remove stale label or comment or this will be closed in 30 days.

@github-actions github-actions bot added the stale label Jul 31, 2023
@github-actions
Copy link

This issue was closed because it has been stale for 30 days with no activity.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Aug 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants