docs: move udf examples to docs/examples #33854
Conversation
Summary of ChangesHello @danielclow, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the documentation for User-Defined Functions (UDFs) by centralizing all associated example code files into a new Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request moves UDF example files to a new docs/examples directory and updates the documentation accordingly. The changes are mostly correct, but I've found several issues in the example code files, including critical security vulnerabilities like buffer overflows, memory leaks, and logic bugs. I've also found some copy-paste errors and other mistakes in the documentation markdown file. My review includes suggestions to fix these issues.
| char *des = resultData->buf + sizeof(uint16_t); | ||
| strcpy(des, str); | ||
|
|
||
| // set binary type len | ||
| uint16_t len = strlen(str); | ||
| *((uint16_t *)resultData->buf) = len; |
There was a problem hiding this comment.
Using strcpy here is unsafe and can lead to a buffer overflow. The destination buffer size is limited by the outputtype in CREATE FUNCTION (e.g., 64 bytes), but the source string can be up to 256 bytes. You must use a bounded copy function like memcpy and respect the destination buffer's capacity.
| char data[64]; | ||
| memset(data, 0, 64); | ||
| memcpy(data, varDataVal(buf), varDataLen(buf)); |
There was a problem hiding this comment.
This code has a critical buffer overflow vulnerability. A fixed-size buffer data[64] is used, but the varchar data from varDataLen(buf) can be much larger, leading to a buffer overflow in memcpy. You should dynamically allocate memory for data based on the actual length of the string.
Remember to free(data) at the end of the for loop iteration to prevent a memory leak.
| udfTrace("block:%p, processing begins, cols:%d rows:%d", block, block->numOfCols, block->numOfRows); | ||
|
|
||
| float maxValue = *(float *)interBuf->buf; | ||
| char strBuff[STR_MAX_LEN] = "inter1buf"; |
There was a problem hiding this comment.
The strBuff is always initialized to the hardcoded string "inter1buf". This is a bug, as it discards the intermediate string result from previous blocks if no new maximum is found in the current block. strBuff should be initialized from the content of interBuf.
char strBuff[STR_MAX_LEN];
memcpy(strBuff, interBuf->buf + sizeof(float), STR_MAX_LEN);| if len(row) > 1: | ||
| new_state = np.cumsum(row)[-1] | ||
| else: | ||
| new_state = state |
There was a problem hiding this comment.
The logic for calculating the cumulative sum is incorrect. It doesn't accumulate the sum from the previous state (state). Instead, it overwrites new_state with the sum of only the current block's values, and only if the block contains more than one value. This means the function doesn't work as a cumulative sum across blocks and ignores data in blocks with one or zero non-null values.
The logic should add the sum of the current block's values to the existing state.
new_state = state + sum(row)| int32_t deviceIdLen = valDataLen < (STR_MAX_LEN - 1) ? valDataLen : (STR_MAX_LEN - 1); | ||
|
|
||
| strncpy(strBuff, deviceId, deviceIdLen); | ||
| snprintf(strBuff + deviceIdLen, STR_MAX_LEN - deviceIdLen, "_(%d,%d)_%f", j, i, maxValue); |
There was a problem hiding this comment.
There is a risk of string truncation here. If deviceIdLen is close to STR_MAX_LEN, the buffer size for snprintf (STR_MAX_LEN - deviceIdLen) will be too small to hold the formatted string "_(%d,%d)_%f". This will result in a truncated and incorrect output string. You should ensure deviceIdLen is limited to leave enough space for the suffix.
|
|
||
| udfTrace("start aggregation, buflen:%d used:%d", buf->bufLen, bufLen); | ||
| memset(buf->buf, 0, sizeof(float) + STR_MAX_LEN); | ||
| *((float *)buf->buf) = INT32_MIN; |
There was a problem hiding this comment.
Initializing a float with INT32_MIN is not ideal. For correctness and portability, you should use a floating-point constant like -FLT_MAX from <float.h>. You will also need to include <float.h>.
*((float *)buf->buf) = -FLT_MAX;| } | ||
|
|
||
| double sumSquares = *(double*)interBuf->buf; | ||
| int8_t numNotNull = 0; |
There was a problem hiding this comment.
The variable numNotNull is initialized here and incremented on line 66, but its value is never used. It should be removed to avoid confusion and improve code clarity. The increment on line 66 should also be removed.
…dengine into docs/udf-examples-336
copy udf example code from tests into docs/examples for use in udf developer doc