depyronick / clickhouse-client

ClickHouse Client for NodeJS
https://clickhouse.js.org
MIT License
49 stars 11 forks source link

Support of write queries with no returned data? #4

Closed David-Mueller closed 2 years ago

David-Mueller commented 2 years ago

Hi there,

first of all thanks for the development and effort and the latest changes! I'm trying to CREATE MATERIALIZED VIEW via the new POST-request and expect no result from ClickHouse.

The process crashes after the (successful!) creation of the view with the following error:

node_modules/stream-json/Parser.js:112
    return callback(new Error('Parser has expected a value'));

Do you think it's worthwhile to insert something like

if (+JSON.parse(stream.headers?.['x-clickhouse-summary'])?.total_rows_to_read === 0) {
  subscriber.complete();
  return;
}

into https://github.com/depyronick/clickhouse-client/blob/9f0a050fe3c5f81f21773cab0cdd2eaaf1c76ab8/src/client/ClickHouseClient.ts#L135-L150 ?

Or maybe there's a better solution. Would be much appreciated!

Cheers

David-Mueller commented 2 years ago

Just noticed, that passing the jsonStreaming: true option to Parser might prevent the problem while still keeping returning possible results correctly:

const pipeline = stream 
             .pipe(new Parser({jsonStreaming: true})) 
             .pipe(new Pick({ 
                 filter: 'data' 
             })) 
             .pipe(new StreamArray()) 

Read inside stream-json wiki

depyronick commented 2 years ago

Hello, @David-Mueller!

Thanks for the issue and helping the library to grow.

At first glance, it seems like it's good to have jsonStreaming option within parser as it states "It handles empty streams producing no values." in their documentation. But I guess it should be handled differently as there won't be only JSON format supported in the future.

I'll try a few different approaches and will try to find the best way to do this.

Should be done within 12 hours, i'll be releasing this fix as 1.0.9.

Thanks!

depyronick commented 2 years ago

Hey, @David-Mueller!

Turns out it's ok to just use jsonStreaming: true option. I got it ready to publish, but, would you like to contribute this yourself?

David-Mueller commented 2 years ago

Hey, thanks for the honours. Of course, feel free to edit, version bump or anything. I can do it as well upon request :)

depyronick commented 2 years ago

Great! Thanks for contributing!