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

Various batch functions and tests. #25

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Conversation

wehriam
Copy link

@wehriam wehriam commented Feb 16, 2012

I'm not sure of the correct version number for a master merge.

@edevil
Copy link

edevil commented Mar 15, 2012

Is there still a problem with this patch?

@wehriam
Copy link
Author

wehriam commented Mar 19, 2012

@edevil not sure. @thepaul ?

@thepaul
Copy link
Collaborator

thepaul commented Mar 19, 2012

Not one I know of. I need to get a chance to investigate it more thoroughly before committing, though. It's on the radar.

@thobbs
Copy link
Collaborator

thobbs commented Mar 29, 2012

@wehriam I have a few questions and suggestions on the pull request.

First, I think the batch functions for counters are definitely needed and almost good to go. I would lean towards combining batch_add and batch_multikey_add; offer only the multikey functionality, but simply call it batch_add. Thoughts?

I'm a little skeptical about the value added by batch_remove_rows. The batch_remove method already offers something similar but is more powerful. Am I missing an advantage? I have similar thoughts about batch_multikey_insert versus the existing batch_mutate.

@wehriam
Copy link
Author

wehriam commented Apr 2, 2012

I agree batch_multikey_add and batch_add should be consolidated.

It is my understanding that batch_remove requires a user to pass names, start, or finish to create a slice predicate/range, or else no columns are removed. (batch_remove_rows removes entire rows.) If batch_remove can be already be used to remove multiple whole rows, I agree batch_remove_rows should be discarded. Otherwise, batch_remove should be updated to include the removal of whole rows without having to specify column names or ranges.

Looking at the tests it appears batch_multikey_insert duplicates batch_mutate functionality.

@thobbs
Copy link
Collaborator

thobbs commented Apr 2, 2012

I'm not sure why batch_remove even has the start, finish, reverse, or count parameters. Having a SliceRange on a SlicePredicate in a Deletion object isn't allowed. As far as Cassandra is concerned, if names is None, it will delete the entire row.

batch_remove should just create the SlicePredicate itself instead of going through _mkpred() and only set its column_names attribute to whatever is passed in as names. This would give it the correct behavior of deleting the entire row if names is None.

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.

4 participants