Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix merge bugs due to path error #47

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

ucasfl
Copy link
Contributor

@ucasfl ucasfl commented Nov 8, 2024

Changelog category (leave one):

  • Bug Fix (user-visible misbehavior in an official stable release)

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

Fix merge fail with vector index.

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/

@Aed-p
Copy link
Collaborator

Aed-p commented Nov 11, 2024

Hi ucasfl,

Thank you for the PR and for taking the time to address this issue!

To help us understand this fix better, could you please provide a description of the bug, including the specific scenario or conditions that trigger it? Additionally, if you have any test cases to verify the fix, that would be very helpful for ensuring everything works as expected and for preventing similar issues in the future.

Thanks again for your contribution!

@ucasfl
Copy link
Contributor Author

ucasfl commented Nov 11, 2024

@Aed-p Use inverted_row_ids_map_file_path as example.

global_ctx->inverted_row_ids_map_file_path
= global_ctx->new_data_part->getDataPartStorage().getFullPath() + "merged-inverted_row_ids_map" + VECTOR_INDEX_FILE_SUFFIX;

It include the full path of the disk, and writeFile only need to provede the file name:

global_ctx->inverted_row_ids_map_uncompressed_buf = global_ctx->new_data_part->getDataPartStorage().writeFile(
global_ctx->inverted_row_ids_map_file_path, 4096, global_ctx->context->getWriteSettings());

std::unique_ptr<WriteBufferFromFileBase> DataPartStorageOnDiskFull::writeFile(
const String & name,
size_t buf_size,
WriteMode mode,
const WriteSettings & settings)
{
if (transaction)
return transaction->writeFile(fs::path(root_path) / part_dir / name, buf_size, mode, settings, /* autocommit = */ false);
else
return volume->getDisk()->writeFile(fs::path(root_path) / part_dir / name, buf_size, mode, settings);
}

So we should use fileName to get the file name when calling writeFile like other places, otherwise, the path pass into Disk become full_disk_path/full_disk_path/filename, leading to file does not exist error due to directory does not exist.

auto rows_sources_read_buf = std::make_unique<CompressedReadBufferFromFile>(ctx->tmp_disk->readFile(fileName(ctx->rows_sources_file->path())));

ctx->rows_sources_uncompressed_write_buf = ctx->tmp_disk->writeFile(fileName(ctx->rows_sources_file->path()), DBMS_DEFAULT_BUFFER_SIZE, WriteMode::Rewrite, global_ctx->context->getWriteSettings());

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.

2 participants