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

Requires a 4-parameter subbits function #12587

Open
tigercl opened this issue Feb 26, 2024 · 5 comments
Open

Requires a 4-parameter subbits function #12587

tigercl opened this issue Feb 26, 2024 · 5 comments
Assignees

Comments

@tigercl
Copy link
Member

tigercl commented Feb 26, 2024

What happened?

Arg 5 and Arg 6 of the rule engine built-in function subbits are only valid when Arg 4 is integer, but the rule engine does not provide a 4-parameter subbits, so when Arg4 is bits or float, I still have to write like this:

subbits('abc', 9, 10, 'bits', 'signed', 'little')
subbits('abc', 9, 10, 'float', 'signed', 'little')

What did you expect to happen?

Support:

subbits('abc', 9, 10, 'bits')
subbits('abc', 9, 10, 'float')

How can we reproduce it (as minimally and precisely as possible)?

No response

Anything else we need to know?

No response

EMQX version

EMQX v5.4.1

OS version

macOS 13.4

Log files

@tigercl tigercl added the BUG label Feb 26, 2024
@id id added the internal label Mar 4, 2024
@kjellwinblad kjellwinblad self-assigned this Mar 5, 2024
kjellwinblad added a commit to kjellwinblad/emqx that referenced this issue Mar 5, 2024
The documentation for the family of subbits functions says that the
fifth and sixth parameters are optional (since they only make sense when
the forth parameter is 'integer'). However, before this commit
`subbits/4` and `subbits/5` did not exist.

Fixes:
https://emqx.atlassian.net/browse/EMQX-11942
emqx#12587
@kjellwinblad
Copy link
Contributor

PR with fix has been created:

#12652

@tigercl
Copy link
Member Author

tigercl commented Mar 14, 2024

@kjellwinblad Hi, I revisited the code of this PR once again, and found that there may be some problems in the implementation.

Neither float nor bits types distinguish between signed and unsigned. Instead, they should distinguish between big endian and little endian.

So the 5th param of the subbits/5 should be Endianness.

@tigercl
Copy link
Member Author

tigercl commented Mar 15, 2024

SELECT
    subbits(hexstr2bin('9F4E58'), 1, 16, 'float', 'unsigned', 'big') as t1,
    subbits(hexstr2bin('9F4E58'), 1, 16, 'float', 'signed', 'big') as t2,
    subbits(hexstr2bin('9F4E58'), 1, 16, 'float', 'unsigned', 'little') as t3,
    bin2hexstr(subbits(hexstr2bin('9F4E58'), 1, 16, 'bits', 'unsigned', 'big')) as t4,
    bin2hexstr(subbits(hexstr2bin('9F4E58'), 1, 16, 'bits', 'signed', 'big')) as t5,
    bin2hexstr(subbits(hexstr2bin('9F4E58'), 1, 16, 'bits', 'unsigned', 'little')) as t6
FROM "t/#"

Result:

{
    "t1": -0.00713348388671875,
    "t2": -0.00713348388671875,
    "t3": 26.484375,
    "t4": "9F4E",
    "t5": "9F4E",
    "t6": "9F4E"
}

Supplement: float distinguishes big and little endian, bits does not distinguish.

@kjellwinblad
Copy link
Contributor

The argument list order is the same in subbits/4 and subbits/5 that I added as it is in subbits/6. This is also the order that is documented https://www.emqx.io/docs/en/latest/data-integration/rule-sql-builtin-functions.html#bit-sequence-functions .

I think we can still change subbits/5 as this function has not existed before (so we know that no one is using it) if we also document clearly that the order differ. However, I don't think this is a good idea. It is a bit strange to have a function with the same name and different arity, but where the argument list order differ. I think this will lead to more confusion and errors than the potential gain of sometimes not having to type one unnecessary argument. Of course, it would be be better if the order between the two last parameters in subbits/6 was swapped and subbits/5 had endianness as the fifth parameter, but unfortunately I think this is too late now since subbits/6 has been released and documented so it might already be in use in production systems.

Supplement: float distinguishes big and little endian, bits does not distinguish.

I guess this is how it should be? I'm not sure if we can change this now since the function has existed in earlier releases.

@tigercl
Copy link
Member Author

tigercl commented Mar 16, 2024

I agree with this.

but unfortunately I think this is too late now since subbits/6 has been released and documented so it might already be in use in production systems.

I think we could remove subbits/5, it just seems to make the situation more complicated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants