Added condtional type to improve typing in case of object row format

[supermar1010]

Hello,
when using the rowFormat: "object" the type for rows in the onComplete still is any[][]. It should be any[].
This PR converts the interface to a type and thus makes it possible to use conditional types.
With this change the type definition of onComplete changes depending on if rowFormat is set to object.

Have a great day!
Mario

[platypii]

I like the type change, but please run lint and typescript, there's some issues to fix please.

[supermar1010]

Ohh sorry, I did a bit of a sloppy job yesterday.
I fixed the issues lint, test and build:types run successful now!

I added two types, one with the rowFormat being optional, which leads to any[][] (but typed as "array", so rowFormat is always available on the type and additionally now "forced" to be either undefined, "array" or "object"). The other one with rowFormat set to object which makes onComplete type: any[]

[severo]

In fact, I think we want to have both fields, or none (rowFormat is useless if onComplete is undefined, and we want to have the rowFormat for type safety of onComplete)

So: maybe set both fields as always defined (remove ?: -> :) here and in ParquetObjectRowReadOptions, and we would have:

export type ParquetReadOptions =  ParquetBaseReadOptions | ParquetArrayRowReadOptions | ParquetObjectRowReadOptions

Or

interface ParquetRowReadOnCompleteArray {
  rowFormat: "array" // format of each row passed to the onComplete function
  onComplete: (rows: any[][]) => void // called when all requested rows and columns are parsed
}
interface ParquetRowReadOnCompleteObject {
  rowFormat: "object" // format of each row passed to the onComplete function
  onComplete: (rows: Record<string, any>[]) => void // called when all requested rows and columns are parsed
}
type ParquetRowReadOnComplete = ParquetRowReadOnCompleteArray | ParquetRowReadOnCompleteObject | {} 
export type ParquetReadOptions = ParquetBaseReadOptions & ParquetRowReadOnComplete

[severo]

(for reference, the doc on discriminated unions: https://www.typescriptlang.org/docs/handbook/typescript-in-5-minutes-func.html#discriminated-unions)

[severo]

Or more compact

type ParquetRowReadOnComplete =
  | {}
  | { rowFormat: "array", onComplete: (rows: any[][]) => void }
  | { rowFormat: "object", onComplete: (rows: Record<string, any>[]) => void }

[severo]

(ups, sorry, I mixed comments and review)

[supermar1010]

I think we should leave onComplete as optional, as there are also places in the code where it is not needed.

Actually here the type is a bit misleading, as you can't pass onComplete as it will always be overwritten

/**
 * @param {ParquetReadOptions} options
 * @returns {Promise<Record<string, any>[]>} resolves when all requested rows and columns are parsed
*/
export function parquetReadObjects(options) {
  return new Promise((onComplete, reject) => {
    parquetRead({
      ...options,
      rowFormat: 'object',
      onComplete,
    }).catch(reject)
  })
}

[severo]

If we do that, we would have to add checks in some places. For example, in

hyparquet/src/read.js

Line 53 in 3c5dbab

if (onComplete) onComplete(rowData)

onComplete will receive any[][], so we would have to check that rowFormat === "array" and throw if not

[severo]

(for reference, the doc on discriminated unions: https://www.typescriptlang.org/docs/handbook/typescript-in-5-minutes-func.html#discriminated-unions)

[severo]

Or more compact

type ParquetRowReadOnComplete =
  | {}
  | { rowFormat: "array", onComplete: (rows: any[][]) => void }
  | { rowFormat: "object", onComplete: (rows: Record<string, any>[]) => void }

[supermar1010]

I think this is how it is supposed to be, but I'm not entirely sure, tests, ts and linting runs successful thoug, manual tests, also look good

[supermar1010]

This adds ts checks to the test suite