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

General disagreement about content filter expressions and expression parameters #37

Open
jrw972 opened this issue Apr 2, 2024 · 7 comments

Comments

@jrw972
Copy link
Contributor

jrw972 commented Apr 2, 2024

Executable's name

  • Publisher: opendds-3.27.0_shape_main_linux, connext_dds-6.1.2_shape_main_linux, eprosima_fastdds-2.13.3_shape_main_linux, toc_coredx_dds-6.0.0_shape_main_linux
  • Subscriber: opendds-3.27.0_shape_main_linux, connext_dds-6.1.2_shape_main_linux, eprosima_fastdds-2.13.3_shape_main_linux, toc_coredx_dds-6.0.0_shape_main_linux

Reproducing the problem

  • Test Suite: rtps_test_suite_1
  • Test Case: Test_Color_0
  • Link to the GitHub Action workflow run: NA

What is the problem?

  • Publisher expected code: OK
  • Publisher produced code: OK
  • Subscriber expected code: OK
  • Subscriber produced code: OK/DATA_NOT_RECEIVED

Suggestions about why this problem exists

The filter expression and expression parameter offered by the various subscribers is thus:

Executable Filter Expression Expression Parameter
opendds-3.27.0_shape_main_linux color = %0 BLUE
connext_dds-6.1.2_shape_main_linux color MATCH %0 'BLUE'
eprosima_fastdds-2.13.3_shape_main_linux color = %0 'BLUE'
toc_coredx_dds-6.0.0_shape_main_linux color = %0 BLUE

In the case of opendds as publisher and connext as subscriber, opendds does not support a MATCH operator so it performs no publisher-side content filtering, i.e., all samples are sent to the subscriber which then would filter again.
This is interoperable but probably not what is intended.
The reverse case succeeds.

In the case of opendds as publisher and eprosima as subscriber, the difference in quoting the parameter causes failure.
The reverse case succeeds.

opendds and coredx agree on the expression and expression parameter so both directions succeed.

I did not test other vendor combinations.

I suggest that the filter expression always be color = %0.

Expression parameters are defined in 2.2.2.3.3 and 2.2.2.3.4 as

The expression_parameters attribute is a sequence of strings that give values to the ‘parameters’ (i.e., "%n" tokens) in
the subscription_expression. The number of supplied parameters must fit with the requested values in the
subscription_expression (i.e., the number of %n tokens).

Annex B implies (clarification is needed) that an expression parameter corresponds to the Parameter non-terminal which can be either an INTEGERVALUE, CHARVALUE, FLOATVALUE, STRING, ENUMERATEDVALUE, or PARAMETER.
Logically, PARAMETER is excepted as the goal of a expression parameter is to supply a value for PARAMETER that appears in an expression as quoted above.
As it relates to the issue at hand, CHARVALUE and STRING require single quotes as delimiters.
This makes sense and is necessary when they appear in an expression.
However, expression parameters are conveyed through string sequences so the use of delimiters is superfluous.
Furthermore, it prevents using the single quote in an expression parameter.
I suggest that expression parameters not be quoted and DDS Specification be clarified on this point.
Thus, for this test, the expression parameter would be BLUE.

Other comments

@jrw972
Copy link
Contributor Author

jrw972 commented Apr 2, 2024

Spec issue is https://issues.omg.org/browse/DDS15-319

@mitza-oci
Copy link
Contributor

mitza-oci commented Apr 3, 2024

I'm wondering about

Annex B implies (clarification is needed) that an expression parameter corresponds to the Parameter non-terminal

The spec uses the term "parameter" in a few different ways. It's clear to me that what it calls the "token" PARAMETER (all-caps) is a distinct entity from the non-terminal Parameter. The reader of the spec should not assume that the data passed to the set_expression_parameters DDS API (and its equivalents) follow any part of this grammar. In fact, the start of section B.1 makes this explicit:

A subset of SQL syntax is used in several parts of the specification:
• The filter_expression in the ContentFilteredTopic (see 2.2.2.3.3, ContentFilteredTopic Class).
• The topic_expression in the MultiTopic (see 2.2.2.3.4, MultiTopic Class [optional]).
• The query_expression in the QueryReadCondition (see 2.2.2.5.9, QueryCondition Class).

@MiguelCompany
Copy link
Contributor

I've thought of a workaround for this: choosing a different field.
We could, for instance, filter on the shape size, since we already have a program argument to set it.
So we could give a meaning to -z in the subscriber:

-z Meaning Filter Expression Expression Parameter
not present or 0 Do not filter
> 0 filter for equality size = %0 <arg_value>
< 0 filter for less than size < %0 <arg_value>

@mitza-oci
Copy link
Contributor

From a user's perspective, the interop test produced an important outcome -- it revealed that various products are not interoperable with respect to this feature. The question now is what to do about it. Avoiding the feature that's non-interoperable reduces the utility of the interop test.

@angelrti
Copy link
Contributor

angelrti commented Sep 6, 2024

We added a section about considerations where I put information about the CFT expression: https://omg-dds.github.io/dds-rtps/test_description.html#considerations-per-product

We are not currently testing the behavior of a CTF wrt writer-side filtering.

In the case of Connext, we support both operands (MATCH and =). So I changed the filter to use = https://github.com/omg-dds/dds-rtps/blob/master/srcCxx/shape_main.cxx#L1002

Currently the only difference is the single quote around the string value. Probably we can create an issue for the spec and close this issue, what do you think?

@mitza-oci
Copy link
Contributor

I agree that having it in documentation is important. And now that the documentation is referenced in this issue, hopefully others will find it when needed.

I don't think the spec needs to change what it's requiring -- as I wrote above I think that's OK. However the spec may need to change to be more clear to readers. It would probably be good to hear from those who read the spec differently than I did, if they're available.

@GerardoPardo
Copy link
Contributor

Agreed with @mitza-oci the test did uncover an incompatibility resulting from an ambiguity in the DDS spec.
I added a comment a related issue in the DDS 1.5 RTF https://issues.omg.org/browse/DDS15-320.

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

No branches or pull requests

5 participants