[client-v2] Allow QuerySettings in POJO queryAll overloads

Open
#3,112 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Assessment

Difficulty
3/5
Estimated time
1-2 days
Newbie friendliness
68/100
Issue type
Feature
Clarity
Mostly clear
Activity status
Active
Tech stack
clickhouse, java
Domain
api

Research direction

Locate the experimental client-v2 POJO queryAll overloads and the existing GenericRecord queryAll overload that accepts QuerySettings. Read those entry points first, then inspect the surrounding queryAll tests. Done means both proposed POJO overloads accept copied settings while still forcing RowBinaryWithNamesAndTypes, with coverage for the supported settings and deserialization behavior.

Written by the indexing model from the issue text.

Description

Hi! This is my first issue in this repository, so apologies if I did not formulate the idea perfectly.

I'm also not completely sure whether this API change is technically appropriate or possible. There may be a reason why QuerySettings cannot or should not be passed to the POJO queryAll overloads. If so, I would be happy to understand the reasoning.

Description

The experimental POJO queryAll API currently does not provide an overload that accepts QuerySettings.

Current overloads:

queryAll(String sqlQuery, Class<T> clazz, TableSchema schema)

queryAll(
    String sqlQuery,
    Class<T> clazz,
    TableSchema schema,
    Supplier<T> allocator
)

Internally, the method creates its own QuerySettings:

QuerySettings settings =
    new QuerySettings()
        .setFormat(ClickHouseFormat.RowBinaryWithNamesAndTypes);

Because of that, callers cannot provide query/server settings such as:

  • limit
  • page
  • max_execution_time
  • query id
  • database
  • other operation-level server settings

At the same time, queryAll for GenericRecord already accepts QuerySettings and copies the supplied settings while forcing the response format to RowBinaryWithNamesAndTypes.

It would be useful if the POJO overloads supported the same behavior.

Proposed API

For example:


public <T> List<T> queryAll(
    String sqlQuery,
    Class<T> clazz,
    TableSchema schema,
    QuerySettings settings
)

public <T> List<T> queryAll(
    String sqlQuery,
    Class<T> clazz,
    TableSchema schema,
    QuerySettings settings,
    Supplier<T> allocator
)

The response format should still be controlled internally by queryAll, since POJO deserialization depends on RowBinaryWithNamesAndTypes.

In other words, the provided settings could be copied and then:

requestSettings.setFormat(
    ClickHouseFormat.RowBinaryWithNamesAndTypes
);

This would keep POJO deserialization safe while allowing callers to use settings such as pagination, execution limits, query id, database, etc.

Why

This would make the POJO queryAll API consistent with the existing GenericRecord queryAll API and would avoid forcing callers to reimplement the POJO deserialization path only to pass custom QuerySettings.

Dominant language
Java
Stars
1.6k
Forks
637
Avg merge
2d 12h
Merged PRs (30d)
28

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

More from ClickHouse/clickhouse-java

All issues in ClickHouse/clickhouse-java

Similar issues

More Java issues

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.