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

StreamEntry support Binary #3803

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

thachlp
Copy link
Contributor

@thachlp thachlp commented Apr 6, 2024

Issue: #3566

Copy link
Collaborator

@sazzad16 sazzad16 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We must not break the existing implementations.

@thachlp
Copy link
Contributor Author

thachlp commented Apr 6, 2024

We must not break the existing implementations.

Hi, @sazzad16, thanks for the review 🙇
How do we resolve this issue?

@sazzad16
Copy link
Collaborator

sazzad16 commented Apr 6, 2024

How do we resolve this issue?

By creating new methods.


I just realized what your title says:

StreamEntry support Binary instead of String

Actually we want StreamEntry binary support while keeping the String ones.

@thachlp
Copy link
Contributor Author

thachlp commented Apr 6, 2024

How do we resolve this issue?

By creating new methods.

I just realized what your title says:

StreamEntry support Binary instead of String

Actually we want StreamEntry binary support while keeping the String ones.

Thanks, I will update.

@sazzad16 sazzad16 changed the title StreamEntry support Binary instead of String StreamEntry support Binary Apr 9, 2024
Copy link
Contributor Author

@thachlp thachlp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @sazzad16, as I understand, there is xadd support Map<byte[], byte[]> hash already.

  public final CommandObject<byte[]> xadd(byte[] key, XAddParams params, Map<byte[], byte[]> hash) {
    return new CommandObject<>(addFlatMapArgs(commandArguments(XADD).key(key).addParams(params), hash),
        BuilderFactory.BINARY);
  }

@sazzad16
Copy link
Collaborator

@thachlp StreamEntry has nothing to do with XADD.

@thachlp thachlp marked this pull request as draft April 19, 2024 14:16
@thachlp thachlp force-pushed the stream-entry-support-binary branch 2 times, most recently from 1ffa1d7 to 431f721 Compare April 19, 2024 14:44
@thachlp thachlp closed this Jun 27, 2024
@jfloodnet
Copy link

jfloodnet commented Jul 21, 2024

Now that this package supports binary value in stream entries, would someone be able to update spark redis to support binary columns also?

It is currently only Map<string, string> https://github.com/RedisLabs/spark-redis/blob/master/src/main/scala/com/redislabs/provider/redis/streaming/RedisStreamReceiver.scala#L108

Which causes the following error:

java.lang.ClassCastException: class org.apache.spark.unsafe.types.UTF8String cannot be cast to class [B (org.apache.spark.unsafe.types.UTF8String is in unnamed module of loader 'app'; [B is in module java.base of loader 'bootstrap') at org.apache.spark.sql.catalyst.expressions.BaseGenericInternalRow.getBinary(rows.scala:46)

If your stream entry schema has binary data:

    public static StructType redisSchema() {
        return new StructType(new StructField[]{
                DataTypes.createStructField("msg", DataTypes.BinaryType, true)
        });
    }

@sazzad16
Copy link
Collaborator

@jfloodnet This PR is closed by OP without being merged.

@jfloodnet
Copy link

@jfloodnet This PR is closed by OP without being merged.

oh that's true. @thachlp what was the reason you closed this PR?

@thachlp
Copy link
Contributor Author

thachlp commented Jul 26, 2024

@jfloodnet I am just ambiguous about doing it, but If it is important, I will continue on it now, I will ask @sazzad16 for help.

@thachlp thachlp reopened this Jul 26, 2024
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.

3 participants