CQL 3.0 with C* 1.1.0~rc1 Tests Fail #42

Closed
devdazed opened this Issue Apr 20, 2012 · 8 comments

Comments

Projects
None yet
3 participants
Owner

devdazed commented Apr 20, 2012

Tests are failing for delete and select by row using C* 1.1.0~rc1 for details see: http://travis-ci.org/#!/simplereach/helenus/jobs/1139784

Contributor

ctavan commented Apr 22, 2012

This was introduced by bd676d4.

The query being executed is

SELECT * FROM 'cql_test' WHERE id='foobar'

but the correct query would be

SELECT * FROM cql_test WHERE id='foobar'

Apparently C* 1.1 / CQL3 does not allow quoting of CF-names any more.

Contributor

ctavan commented Apr 22, 2012

I think we should not try to solve this programmatically and just reserve placeholder-replacement to ColumnNames and ColumnValues. This seems to be a common approach with other databases, that use a query-language, like (again) node-mysql.

I'll provide a fix in a minute.

Contributor

ctavan commented Apr 22, 2012

What a stupid bug: I was not defining the timezone explicitly in CQL date-strings which caused the tests to work on my local laptop but made them fail in travis...

Now works all well :).

@devdazed devdazed closed this in 7adac77 Apr 23, 2012

This issue seems to affect column names too. I.e., it appears that only column value substitution works now using CQL 3.0. (There doesn't appear to be a test that covers this?) I think it would be nice if placeholders continued to work for column names (and even CF names). Maybe auto quoting could be made optional?

Contributor

ctavan commented May 7, 2012

Technically, as long as you still go with CQL 2, you can still use placeholders for column names and CF names with helenus.

In CQL 3, however, "dynamic" column names as we know them from CQL 2 don't exist any more, hence quoting these names will result in CQL parse errors (I'm not 100% an expert on this but that's how I understand the intention of CQL 3. Please correct me if I'm wrong!).

Placeholders are meant to provide an easy and safe way to insert user-provided data into a query. Since there are no more "dynamic" column names in CQL 3 which actually could be considered data in CQL 2, I think it would be wrong to allow placeholders for column names (same goes for CF names).

Also CQL 3 now has prepared CQL statements wich may contain placeholders and which are stored serverside (not yet implemented in helenus, but I'm working on it). These prepared statements don't allow placeholders for CF or column names neither. I think any query containing a placeholder that is accepted by helenus should potentially also work as a prepared statement, so allowing un-quoted placeholders in the case where helenus replaces the placeholders will probably cause a lot of unexpected trouble when trying to run the same queries as prepared statements.

Contributor

ctavan commented May 7, 2012

Ah, you mentioned that there is not test covering the behavior described above: That's right, we leave it to the developer to write proper queries that work with the target version of CQL since assuring the correct usage of placeholders in the queries would mean that we would have to re-implement the entire CQL-parser within helenus which IMO makes not much sense.

Thanks for your response.

Ok, fair enough. I'm certainly no expert on CQL 3 myself. It seems to me that even if CQL 3 does not allow dynamic column names, the client side library could support placeholders as a programming convenience (i.e, where the specified column name must be one that exists). Of course the client application could implement something itself to support dynamically generated queries with variable column names if so desired. If you feel it's better not to support placeholders for column names because it goes against the intent of CQL 3, I'm ok with that.

Note, however, that you stated in a previous comment, "... and just reserve placeholder-replacement to ColumnNames and ColumnValues", which did lead me to believe the intent was to support placeholders for column names.

Also note that the CQL example on your main page:

  pool.cql("SELECT '%s' FROM cf_one WHERE key='%s'", ['col','key123'], function(err, results){
    console.log(err, results);
  });

is broken for CQL 3. It might be worth adding a comment to that effect, to save some time for people who are trying it out.

ctavan added a commit to ctavan/helenus that referenced this issue May 7, 2012

Account for CQL3 changes in Readme, see #42
- Change example to be CQL 3 compliant.
- Don't mention %s as placeholder any more (since it wouldn't be
  compatible with CQL 3 prepared statements).
- Also mention that you cannot use placeholders for Column names
Contributor

ctavan commented May 7, 2012

@rmechler I admit my previous comment was mis-leading. I wrote it before actually fixing the problem and before I found out, that dynamical column names are no longer supported in CQL 3. Please regard that as obsolete.

And yes, the example in the Readme is definitely CQL 2 syntax and won't work with CQL 3 anymore. On the other hand CQL 3 is still beta and will not become the default before Cassandra 1.2, so I'm not sure whether it's already the right time to re-write the documentation. However a comment would be useful, you're right. See #46

devdazed added a commit that referenced this issue May 7, 2012

Merge pull request #46 from ctavan/add-cql3-notes-to-readme
Account for CQL3 changes in Readme, see #42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment