feat: export queryFieldInfo and jsType #625

kwonoh · 2024-12-04T16:42:01Z

export two utils functions queryFieldInfo and jsType

jheer · 2024-12-04T17:50:11Z

Thanks @kwonoh. This looks like a reasonable and straightforward modification. Can you share a bit more about why you'd like to have these methods exported? (It helps us to keep track of various use cases!)

kwonoh · 2024-12-04T17:59:32Z

Hi @jheer,

queryFieldInfo: I will use this for a catalog feature in my application to provide information about multiple tables (e.g., get what tables are available via information_schema.tables and use queryFieldInfo to get the column information of each table)
jsType: I plan to use this to determine expected JS types returned from some dynamically built queries. Also, I might use this function for the catalog feature purpose as well (e.g., use against data_type column of information_schema.columns).

jheer

Overall looks good. If we're going to expose the queryFieldInfo method, we should also add jsdoc comments and types before merging.

kwonoh · 2024-12-04T20:49:04Z

Addressing test failures

kwonoh · 2024-12-05T03:06:14Z

@jheer I have added the jsdoc and type definitions. Now, it passes the tests, but please review the types closely. Some notes and questions:

I am not sure FieldInfoRequest is a good name for the return type of MosaicClient.fields and the type of fields param of queryFieldInfo
Could you explain where column.aggregate is set in the getFieldInfo function? I have added & { aggregate?: boolean } part in FieldInfoRequest for this usecase, but I couldn't find where it's coming from.
The params of summarize has been updated to use FieldInfoRequest.
Stats in field-info.js seems not being used. What's the plan for this variable?

kwonoh · 2024-12-05T03:10:08Z

packages/core/src/Coordinator.js

@@ -27,7 +27,7 @@ export function coordinator(instance) {
 }

 /**
- * @typedef {import('@uwdata/mosaic-sql').Query | string} QueryType
+ * @typedef {import('@uwdata/mosaic-sql').Query | import('@uwdata/mosaic-sql').DescribeQuery | string} QueryType


DescribeQuery was missing as a QueryType causing an error

kwonoh · 2024-12-05T03:11:37Z

packages/core/src/util/field-info.js

+ *   table: string | import('@uwdata/mosaic-sql').TableRefNode,
+ *   column: (string | import('@uwdata/mosaic-sql').ColumnRefNode) & { aggregate?: boolean },
+ *   stats?: Stat[] | Set<Stat>
+ * }} FieldInfoRequest


Not sure FieldInfoRequest is a good name for a type that represents the return type of MosaicClient.fields and the type of fields param of queryFieldInfo

kwonoh · 2024-12-05T03:13:24Z

packages/core/src/util/field-info.js

+ * 
+ * @typedef {{
+ *   table: FieldInfoRequest["table"],
+ *   column: string,


While FieldInfoRequest.column is string | ColumnRefNode, both getFieldInfo and getTableInfo return only string type for column field.

kwonoh · 2024-12-05T03:18:41Z

packages/core/src/Coordinator.js

@@ -134,18 +134,20 @@ export class Coordinator {
   *  or a SQL string.
   * @param {object} [options] An options object.
   * @param {'arrow' | 'json'} [options.type] The query result format type.
-   * @param {boolean} [options.cache=true] If true, cache the query result.
+   * @param {boolean} [options.cache=true] If true, cache the query result in the client (QueryManager).
+   * @param {boolean} [options.persist=false] If true, persist cached query result in the server.


persist option was missing in the type definition causing an error

Can you say more about where the error was coming from? I believe persist should be part of the aggregated ...options, not broken out individually.

Can you say more about where the error was coming from?

The type error was from { persist: true } ingetFieldInfo. Currently, there is no persist field in the options type info of Coordinator.query here.

I believe persist should be part of the aggregated ...options, not broken out individually.

~~persist type info is added as a member of options object (options.persist not persist), not as a separate field. Do you want to add a new interface type definition for options separately?~~

~~or did you mean there should be no default value assignment for persist?~~

I now see what you mean. Updated the code.

kwonoh · 2024-12-05T03:58:08Z

packages/core/src/Coordinator.js

   * @param {number} [options.priority] The query priority, defaults to
   *  `Priority.Normal`.
   * @returns {QueryResult} A query result promise.
   */
  query(query, {
    type = 'arrow',
    cache = true,
+    persist = false,


The default value of persist differs depending on the server implementation

duckdb-server-rust: true cc @domoritz

duckdb-server (Python): false

duckdb (Node.js): false

persist = false, has been removed as it needs to be part of ...options.
But that means the default value of persist should be the same across the servers

jheer · 2024-12-13T18:09:16Z

packages/core/src/Coordinator.js

@@ -134,18 +134,20 @@ export class Coordinator {
   *  or a SQL string.
   * @param {object} [options] An options object.
   * @param {'arrow' | 'json'} [options.type] The query result format type.
-   * @param {boolean} [options.cache=true] If true, cache the query result.
+   * @param {boolean} [options.cache=true] If true, cache the query result in the client (QueryManager).
+   * @param {boolean} [options.persist=false] If true, persist cached query result in the server.


Can you say more about where the error was coming from? I believe persist should be part of the aggregated ...options, not broken out individually.

jheer · 2024-12-13T20:30:35Z

Thanks!

jheer self-requested a review December 4, 2024 19:07

jheer requested changes Dec 4, 2024

View reviewed changes

kwonoh requested a review from jheer December 5, 2024 03:06

kwonoh commented Dec 5, 2024

View reviewed changes

jheer requested changes Dec 13, 2024

View reviewed changes

kwonoh requested a review from jheer December 13, 2024 19:32

kwonoh added 8 commits December 13, 2024 15:19

feat: export queryFieldInfo and jsType

02d64ee

fix typo

03e71a7

feat: add jsdoc comments and type definitions for field info

dc6efa5

fix the type of FieldInfo.table

7147772

refine types

a3e47c2

update comment about options.persist

3e0647a

clarify the difference between cache and persist

ef45fcc

persist should be part of rest of options

1bbfb88

kwonoh force-pushed the export-query-field-info-and-js-type branch from 8f4d75f to 1bbfb88 Compare December 13, 2024 20:21

jheer approved these changes Dec 13, 2024

View reviewed changes

jheer merged commit dc41a9a into uwdata:main Dec 13, 2024
3 checks passed

kwonoh deleted the export-query-field-info-and-js-type branch December 16, 2024 15:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: export queryFieldInfo and jsType #625

feat: export queryFieldInfo and jsType #625

kwonoh commented Dec 4, 2024

jheer commented Dec 4, 2024

kwonoh commented Dec 4, 2024 •

edited

Loading

jheer left a comment

kwonoh commented Dec 4, 2024

kwonoh commented Dec 5, 2024

kwonoh Dec 5, 2024

kwonoh Dec 5, 2024

kwonoh Dec 5, 2024 •

edited

Loading

kwonoh Dec 5, 2024

jheer Dec 13, 2024

kwonoh Dec 13, 2024 •

edited

Loading

kwonoh Dec 13, 2024

kwonoh Dec 5, 2024

kwonoh Dec 13, 2024

jheer Dec 13, 2024

jheer commented Dec 13, 2024

feat: export queryFieldInfo and jsType #625

feat: export queryFieldInfo and jsType #625

Conversation

kwonoh commented Dec 4, 2024

jheer commented Dec 4, 2024

kwonoh commented Dec 4, 2024 • edited Loading

jheer left a comment

Choose a reason for hiding this comment

kwonoh commented Dec 4, 2024

kwonoh commented Dec 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwonoh Dec 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwonoh Dec 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jheer commented Dec 13, 2024

kwonoh commented Dec 4, 2024 •

edited

Loading

kwonoh Dec 5, 2024 •

edited

Loading

kwonoh Dec 13, 2024 •

edited

Loading