Skip to content

feat: Support table view for C client. #294

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

Merged
merged 5 commits into from
Jul 7, 2023

Conversation

shibd
Copy link
Member

@shibd shibd commented Jun 27, 2023

Motivation

Support table view for C client.

Modifications

  • Add table_view for C client.
  • Add table_view_configuration for C client.

Verifying this change

  • Add CTableViewTest cover it.

Documentation

  • doc-required
    (Your PR needs to update docs and you will update later)

  • doc-not-needed
    (Please explain why)

  • doc
    (Your PR contains doc changes)

  • doc-complete
    (Docs have been already added)

@shibd shibd added this to the 3.3.0 milestone Jun 27, 2023
@shibd shibd self-assigned this Jun 27, 2023
@BewareMyPower
Copy link
Contributor

In addition, I don't think use a null-terminated string as the tableview value is good for C API. The value is actually a byte array rather than a null-terminated string. Unlike the TableView class, it's okay to return a std::string, which is actually a byte array in C++. For example, if the producer sends 4 bytes: [0x01, 0x00, 0x02, 0x00], you can never know the length.

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

static void get_value(char **value) {
  char *data = (char *)malloc(4);
  data[0] = 0x01;
  data[1] = 0x00;
  data[2] = 0x02;
  data[3] = 0x00;
  *value = data;
}

int main(int argc, char *argv[]) {
  char *value;
  get_value(&value);
  printf("length: %zu\n", strlen(value));
  return 0;
}

The code example is a demo to show the difference between a byte array and a null-terminated string.

So it's better to add a length parameter like:

#include <stdio.h>
#include <stdlib.h>

static void get_value(char **value_ptr, size_t *value_len) {
  char *data = (char *)malloc(4);
  data[0] = 0x01;
  data[1] = 0x00;
  data[2] = 0x02;
  data[3] = 0x00;
  *value_ptr = data;
  *value_len = 4;
}

int main(int argc, char *argv[]) {
  char *value;
  size_t len;
  get_value(&value, &len);
  printf("length: %zu\n", len);
  for (size_t i = 0; i < len; i++) {
    printf("0x%02x%c", value[i], (i < len - 1) ? ' ' : '\n');
  }
  return 0;
}

Output:

length: 4
0x01 0x00 0x02 0x00

@shibd
Copy link
Member Author

shibd commented Jul 5, 2023

In addition, I don't think use a null-terminated string as the tableview value is good for C API. The value is actually a byte array rather than a null-terminated string. Unlike the TableView class, it's okay to return a std::string, which is actually a byte array in C++. For example, if the producer sends 4 bytes: [0x01, 0x00, 0x02, 0x00], you can never know the length.

Thanks explain, that makes sense to me.

New commit modifications:

  1. Add *value_size param on pulsar_table_view_get_value, pulsar_table_view_retrieve_value method and pulsar_table_view_action.
  2. Change *value type from char **value to void **value.
  3. Remove pulsar_string_map_t *pulsar_table_view_snapshot(pulsar_table_view_t *table_view) method. Because on this case.
  • pulsar_string_map_get also does not return value_size,
  • this string_map may not be suitable for table_view for C.
  • I think the table_view currently provided interface can meet the user use case.
  • If needed, I'll add it in another PR.

PTAL.

@shibd shibd requested a review from BewareMyPower July 5, 2023 09:47
@shibd shibd requested a review from BewareMyPower July 6, 2023 07:59
Copy link
Contributor

@BewareMyPower BewareMyPower left a comment

Choose a reason for hiding this comment

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

LGTM except the example is still wrong.

@shibd shibd requested a review from BewareMyPower July 6, 2023 12:30
@BewareMyPower BewareMyPower merged commit 51d11db into apache:main Jul 7, 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.

2 participants