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

readme: fix create function statement #11

Merged
merged 1 commit into from
Mar 17, 2023

Conversation

wmitros
Copy link
Collaborator

@wmitros wmitros commented Mar 6, 2023

The example CREATE FUNCTION CQL did not include the LANGUAGE clause.
This patch specifies the language to wasm

Pre-review checklist

  • I have split my patch into logically separate commits.
  • All commit messages clearly explain what they change and why.
    - [ ] I added relevant tests for new features and bug fixes.
  • All commits compile, pass static checks and pass tests.
  • PR description sums up the changes and reasons why they should be introduced.
    - [ ] I added appropriate Fixes: annotations to PR description.

@wmitros
Copy link
Collaborator Author

wmitros commented Mar 7, 2023

scylladb/scylladb#13089 is being merged, so I changed "xwasm" to "wasm" here as well

@piodul
Copy link
Collaborator

piodul commented Mar 7, 2023

scylladb/scylladb#13089 is being merged, so I changed "xwasm" to "wasm" here as well

Right. However, if there are some released Scylla versions that only recognize xwasm - even if it is experimental there - then I think we should briefly mention it.

@wmitros
Copy link
Collaborator Author

wmitros commented Mar 7, 2023

scylladb/scylladb#13089 is being merged, so I changed "xwasm" to "wasm" here as well

Right. However, if there are some released Scylla versions that only recognize xwasm - even if it is experimental there - then I think we should briefly mention it.

I see it's present in 5.1 and I think it still won't be updated in 5.2, so quite a lot of time will pass till this reaches an official release. Maybe we should recommend trying this out with Scylla master branch

@piodul
Copy link
Collaborator

piodul commented Mar 7, 2023

scylladb/scylladb#13089 is being merged, so I changed "xwasm" to "wasm" here as well

Right. However, if there are some released Scylla versions that only recognize xwasm - even if it is experimental there - then I think we should briefly mention it.

I see it's present in 5.1 and I think it still won't be updated in 5.2, so quite a lot of time will pass till this reaches an official release. Maybe we should recommend trying this out with Scylla master branch

I don't think many people will be willing to compile Scylla just to play with experimental UDFs. We can just mention that some older versions of Scylla (5.2 and below) use "xwasm" instead of "wasm" as the name of the language - and that's it.

@wmitros
Copy link
Collaborator Author

wmitros commented Mar 7, 2023

I don't think many people will be willing to compile Scylla just to play with experimental UDFs. We can just mention that some older versions of Scylla (5.2 and below) use "xwasm" instead of "wasm" as the name of the language - and that's it.

Makes sense. But in that case maybe we should document this the other way around - the default for now being "xwasm" and we mention "wasm" as the option for Scylla master. We can't really call 5.2 "old" if it isn't even out yet

@piodul
Copy link
Collaborator

piodul commented Mar 7, 2023

Makes sense. But in that case maybe we should document this the other way around - the default for now being "xwasm" and we mention "wasm" as the option for Scylla master. We can't really call 5.2 "old" if it isn't even out yet

Ok, 5.2 is not old yet, I agree. Then let's just drop the "old" epithet and say that ScyllaDB 5.2 and below use a different language name.

I don't think we shoud use "xwasm" in the example, though. We should use "wasm" right now. The example will become correct for the upcoming versions of ScyllaDB. For now it is only slightly inconvenient for those users who want to use UDFs right now and don't read any surrounding text - they will either have to learn to read the surrounding text, or wait for ScyllaDB 5.3. On the other hand, it will be more convenient for us because we won't have to remember to update the language name in the example yet again.

@wmitros
Copy link
Collaborator Author

wmitros commented Mar 8, 2023

Ok, I added a NOTE about Scylla 5.1 and 5.2

The example CREATE FUNCTION CQL did not include the LANGUAGE clause.
This patch specifies the language to wasm
@piodul piodul merged commit 345f563 into scylladb:main Mar 17, 2023
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.

None yet

2 participants