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 compute_mbr for var-sized dimensions #1732

Merged
merged 1 commit into from Jul 22, 2020
Merged

Conversation

joe-maley
Copy link
Contributor

There is a mistake in the logic of compute_mbr and compute_mbr_var that
assumes that the var-sized cells in the value tile do not span more than one
chunked buffer. If a var-sized cell contains 2 integer values, it is only true
that the bytes for an individual integer value do not span a chunked buffer.
The first integer value may be at the end of one chunked buffer and the second
integer may be on the next chunked buffer.

This could be easily fixed, but I also noticed that the write path (where these
functions are called) guarantees that tile chunked buffers will always be
contiguous. In this scenario, we can just revert both compute_mbr and
compute_mbr_var to the logic before the chunked buffer patch (commit hash
4b4b502).

@stavrospapadopoulos
Copy link
Member

In the write path, for sure the values are (or should be) in a contiguous buffer.

HISTORY.md Outdated Show resolved Hide resolved
There is a mistake in the logic of `compute_mbr` and `compute_mbr_var` that
assumes that the var-sized cells in the value tile do not span more than one
chunked buffer. If a var-sized cell contains 2 integer values, it is only true
that the bytes for an individual integer value do not span a chunked buffer.
The first integer value may be at the end of one chunked buffer and the second
integer may be on the next chunked buffer.

This could be easily fixed, but I also noticed that the write path (where these
functions are called) guarantees that tile chunked buffers will always be
contiguous. In this scenario, we can just revert both `compute_mbr` and
`compute_mbr_var` to the logic before the chunked buffer patch (commit hash
4b4b502).
@joe-maley joe-maley merged commit 9d77532 into dev Jul 22, 2020
@joe-maley joe-maley deleted the jpm/fix-compute-mbr-var branch July 22, 2020 14:19
@github-actions
Copy link
Contributor

The backport to release-2.0.7 failed:

The process 'git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-release-2.0.7 release-2.0.7
# Navigate to the new working tree
cd .worktrees/backport-release-2.0.7
# Create a new branch
git switch --create backport-1732-to-release-2.0.7
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick 9d7753249b7a1470e666e2520013f773322e9921
# Push it to GitHub
git push --set-upstream origin backport-1732-to-release-2.0.7
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-release-2.0.7

Then, create a pull request where the base branch is release-2.0.7 and the compare/head branch is backport-1732-to-release-2.0.7.

Shelnutt2 pushed a commit that referenced this pull request Jul 22, 2020
There is a mistake in the logic of `compute_mbr` and `compute_mbr_var` that
assumes that the var-sized cells in the value tile do not span more than one
chunked buffer. If a var-sized cell contains 2 integer values, it is only true
that the bytes for an individual integer value do not span a chunked buffer.
The first integer value may be at the end of one chunked buffer and the second
integer may be on the next chunked buffer.

This could be easily fixed, but I also noticed that the write path (where these
functions are called) guarantees that tile chunked buffers will always be
contiguous. In this scenario, we can just revert both `compute_mbr` and
`compute_mbr_var` to the logic before the chunked buffer patch (commit hash
4b4b502).

Co-authored-by: Joe Maley <joe@tiledb.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants