-
Notifications
You must be signed in to change notification settings - Fork 30
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
Add support for vector type #475
Conversation
@absurdfarce is this covering for JSON based converter too? Courtesy: @eolivelli |
@@ -59,7 +59,7 @@ | |||
make sure the resulting binary tarball contains only | |||
required jars, and that no jar has an offending license. | |||
--> | |||
<driver.version>4.14.1</driver.version> | |||
<driver.version>4.16.0</driver.version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Optional] It may be better to incorporate this Java Driver enhancements
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is definitely required to use 4.16.0 and I would need to have 4.16.1 with this patch
apache/cassandra-java-driver#1643
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re: question from @msmygit ... we can move to a Java driver that includes the changes in the PR you mention but they certainly aren't essential for what we're doing here. It would be a bit nicer to not have to create lots of builder objects for every vector we parse from input but that's a relatively small cost to pay; the JVM is very good at creating (and reaping) lots of small objects. :)
Re: question from @eolivelli .... I agree that 4.16.0 is absolutely required for this change. The constraint on anything beyond 4.16.0 refers to another PR along similar lines. This PR uses 4.16.0 as it stands.
Good question @msmygit. If I were to guess I would say it does not support any JSON-specific functionality. I'm not sure if those represent different code paths or not; I'll dig in some more and check on it. Thanks for the suggestion! This is exactly the kind of feedback I was hoping for from this PR! |
is the Cassandra Pulsar Sink we need JSON capabilities. |
I added support for the JSON stuff here |
@absurdfarce Travis CI is dead, |
Implementation of JSON support added here. This is largely based on work done by @eolivelli here, although I wound up making a number of additional changes as well. |
we should definitely do this. |
TravisCI is still a going concern for a number of projects. I'm not sure why it isn't working for dsbulk but it is very definitely still used by DataStax projects. We also have the Jenkins build, which is really canonical for most cases. We can have a discussion at some point in the future about replacing TravisCI with GH Actions but we have the Jenkins build to handle the immediate need... so I'd definitely say this is future work. |
The recent merge of 1.x into this branch included a fix to get Jenkins working again. The result of that build is indeed very green, although it looks like by default Jenkins largely runs unit tests and a few integration tests. I'll try a re-run that includes all integration tests. It appears that this build will take several hours to complete. |
I'm seeing repeated failures when running the integration tests, not with the tests themselves but with odd download errors when grabbing dependencies. At least this run made it through all the actual test runs, and the absence of a failure of any kind there leaves me pretty confident that we're good on the integration tests front. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After the latest commit 5f23551, all jenkins tests are passing, so I'm going to approve this PR and merge it.
Will work on generating a new release shortly after.
Given the following table:
the changes in this PR enable the following:
It also adds support to the new syntax to the built-in minimal CQL parser which allows for this kind of operation:
Data on the server side matches up to what we'd expect: