Skip to content

Conversation

@yxheartipp
Copy link

Changelog category (leave one):

  • New Feature
  • Improvement
  • Performance Improvement
  • Backward Incompatible Change
  • Build/Testing/Packaging Improvement
  • Documentation (changelog entry is not required)
  • Bug Fix (user-visible misbehavior in an official stable release)
  • Not for changelog (changelog entry is not required)

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

...

Documentation entry for user-facing changes

  • Documentation is written (mandatory for new features)

Information about CI checks: https://clickhouse.com/docs/en/development/continuous-integration/

@baibaichen baibaichen requested a review from Copilot March 25, 2025 07:47
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot wasn't able to review any files in this pull request.

Files not reviewed (6)
  • src/Processors/Formats/Impl/Parquet/ColumnFilterHelper.cpp: Language not supported
  • src/Processors/Formats/Impl/Parquet/ColumnFilterHelper.h: Language not supported
  • src/Processors/Formats/Impl/Parquet/ParquetReader.cpp: Language not supported
  • src/Processors/Formats/Impl/Parquet/ParquetReader.h: Language not supported
  • src/Processors/Formats/Impl/Parquet/RowGroupChunkReader.cpp: Language not supported
  • src/Processors/Formats/Impl/Parquet/RowGroupChunkReader.h: Language not supported

@baibaichen
Copy link

@CodiumAI-Agent /review

@QodoAI-Agent
Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Naming Inconsistency

The function "addCondations" appears to have a typo. It should likely be renamed "addConditions" for clarity and consistency.

void ParquetReader::addCondations(std::unordered_map<String, DataTypePtr> conditions)
{
    std::unordered_set<std::string> header_names;
    for (const auto & col_with_name : header)
    {
        header_names.insert(col_with_name.name);
    }
    for (const auto & [name, type] : conditions)
    {
        if (!header_names.contains(name))
        {
            condition_data_types.emplace(name, type);
        }
    }
}
New Feature Testing

The new method "read_with_select_conditions" has been added along with its integration in related components. Ensure that its behavior is fully tested, covers all edge cases, and integrates correctly with the overall filtering mechanism.

Block ParquetReader::read_with_select_conditions() const
{
    Chunk chunk = chunk_reader->read_with_select_conditions(max_block_size);
    if (!chunk)
        return header.cloneEmpty();
    return header.cloneWithColumns(chunk.detachColumns());
}

void ParquetReader::setSourceArrowFile(std::shared_ptr<arrow::io::RandomAccessFile> arrow_file_)

@baibaichen
Copy link

@CodiumAI-Agent /improve

@QodoAI-Agent
Copy link

PR Code Suggestions ✨

No code suggestions found for the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants