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

[CUBVEC-24] Convert CHAR types to DB Vector in tp_value_cast #5771

Open
wants to merge 13 commits into
base: cubvec/cubvec
Choose a base branch
from
60 changes: 60 additions & 0 deletions src/compat/db_set.c
Original file line number Diff line number Diff line change
Expand Up @@ -296,6 +296,66 @@ db_seq_create (MOP classop, const char *name, int size)
return (set);
}

/*
* db_vec_create() - This function creates an empty vector. The class and
* name arguments can be set to NULL. If values are supplied, a check will
* be made to make sure that the attribute was defined with the vector
* domain.
* return : a set (vector) descriptor
* classop(in): class or instance
* name(in): attribute name
* size(in): initial size
*
* note : The new set will not be attached to any object, so you must use the
* db_put( ) function to assign it as the value of an attribute. If the size
* is not known, it is permissible to pass zero.
*/
DB_SET *
db_vec_create (MOP classop, const char *name, int size)
{
DB_SET *set;
Comment on lines +309 to +316
Copy link
Contributor Author

@vimkim vimkim Jan 6, 2025

Choose a reason for hiding this comment

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

이 부분은 보조 함수이니, 리뷰에 공을 많이 들이지 않으셔도 될 것 같습니다.

기존의 db_seq_create() 함수를 완전히 모방하였으며,

기존에는 set_create_vector() 함수만으로 진행하려 계획이었으나
기존 sequence insert의 콜스택을 분석하면서 db_seq_create 이 호출되는 것을 인지하고,
조금 더 완벽하게 sequence를 모방하고자 db_vec_create() 함수를 생성하기로 했습니다.

#if !defined(SERVER_MODE)
int error = NO_ERROR;
#endif

CHECK_CONNECT_NULL ();

set = NULL;
if (classop == NULL || name == NULL)
{
set = set_create_vector (size);
}
else
{
#if !defined(SERVER_MODE)
SM_CLASS *class_;
SM_ATTRIBUTE *att;

if (au_fetch_class (classop, &class_, AU_FETCH_READ, AU_SELECT) == NO_ERROR)
{
att = classobj_find_attribute (class_, name, 0);
if (att == NULL)
{
ERROR_SET1 (error, ER_OBJ_INVALID_ATTRIBUTE, name);
}
else
{
if (att->type->id == DB_TYPE_VECTOR)
{
set = set_create_vector (size);
}
else
{
ERROR_SET1 (error, ER_OBJ_DOMAIN_CONFLICT, name);
}
}
}
#endif
}

return (set);
}

/*
* db_set_free() - This function frees a set handle. If the set is owned by an
* object, the contents of the set are not freed, only the set handle is
Expand Down
1 change: 1 addition & 0 deletions src/compat/db_set_function.h
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ extern "C"
extern DB_COLLECTION *db_set_create_basic (DB_OBJECT * classobj, const char *name);
extern DB_COLLECTION *db_set_create_multi (DB_OBJECT * classobj, const char *name);
extern DB_COLLECTION *db_seq_create (DB_OBJECT * classobj, const char *name, int size);
extern DB_COLLECTION *db_vec_create (DB_OBJECT * classobj, const char *name, int size);
extern int db_set_free (DB_COLLECTION * set);
extern int db_set_filter (DB_COLLECTION * set);
extern int db_set_add (DB_COLLECTION * set, DB_VALUE * value);
Expand Down
167 changes: 167 additions & 0 deletions src/object/object_domain.c
Original file line number Diff line number Diff line change
Expand Up @@ -611,6 +611,7 @@ static DB_BIGINT tp_ubi_to_bi_with_args (UINT64 ubi, bool is_negative, bool trun
DB_DATA_STATUS * data_stat);

static UINT64 tp_ubi_times_ten (UINT64 ubi, bool * truncated);
static int tp_str_to_vector (DB_VALUE const *src, DB_VALUE * result);

/*
* tp_init - Global initialization for this module.
Expand Down Expand Up @@ -4994,6 +4995,154 @@ tp_atof (const DB_VALUE * src, double *num_value, DB_DATA_STATUS * data_stat)
return status;
}

/*
* tp_str_to_vector - Coerce a string to a vector.
Comment on lines +4999 to +5000
Copy link
Contributor Author

Choose a reason for hiding this comment

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

이 함수를 특히 면밀히 리뷰를 해주시면 감사하겠습니다. 감사합니다.

Copy link
Member

Choose a reason for hiding this comment

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

이 함수의 구현은 db_date.c에 구현되어 있는 db_string_to_XXX 와 비슷한 기능 목적으로 보여서, 동일한 루틴을 가져가도 좋을 것 같습니다. 동일한 파일의 tp_atotimestamptz ()를 참고해서 비슷하게 구현해보면 좋지 않을까요.

다음 두가지에 대해서 검토해볼 수 있을 것 같습니다

  1. db_string_to_vector() 구현 위치
    option 1. src/compat/db_vector.cpp, src/compat/db_vector.hpp 생성 후 db_string_to_vector 구현
    option 2. src/compat/db_set.c, src/compat/db_set.h 에 구현 후 이후에 다른 이슈에서 option 1 진행

option 1 또는 option 2 진행 후에는 이 함수가 잘 정리될 것 같습니다.

  1. intl_skip_spaces () 사용

문자열은 float array를 가지므로, 간단히 '[', [0-9], ']', ',', '-', ' ' 외에는 다른 문자는 담겨있지 않다고 가정하고 있을 것 같습니다. 따라서 모든 문자는 ASCII 영역에 있을 것이므로 intl_skip_spaces ()를 사용하지 않고 isspace ()로 충분해 보입니다.

위 구현으로 충분하려면 intl_skip_spaces ()에서 INTL_CODESET_KSC5601_EUC (= EUC-KR) 에서 확인하고 있는 0xa1a1에 대해서 다음의 결정이 필요해보입니다.

  • vector를 나타내는 문자열에 0xa1a1을 사용 가능한 공백으로 볼지, 에러 문자로 생각할지

이슈 진행 과정에서 예외 사항에 대한 최소한의 확인으로 진행하기로 했기 때문에 제 의견은 0xa1a1에 대해서는 유효하지 않은 문자라고 생각하고 char_isspace() 만으로만 공백을 처리하는 것이 좋을 것 같습니다.

의견 부탁드립니다 :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

정말 중요한 조언 감사드립니다.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. 파일을 분리하는 것이 고민이었는데 아이디어에 감사드립니다.
  2. 최소한의 예외 사항을 처리해야 할지 고민이었는데, 말씀하신 것처럼 기존에 정한 스펙을 따라가겠습니다.

* return: NO_ERROR or error code.
* src(in): string DB_VALUE
* result(out): vector DB_VALUE
* Note:
* Accepts strings that are not null terminated. Don't call this unless
* src is a string db_value.
*/
static int
tp_str_to_vector (const DB_VALUE * src, DB_VALUE * result)
{
const char *p = db_get_string (src);
const char *end = p + db_get_string_size (src);
int count = 0;
const int number_buffer_size = 64;
char number_buffer[number_buffer_size];
int buffer_idx;
Comment on lines +5014 to +5016
Copy link
Contributor Author

Choose a reason for hiding this comment

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

object_domain.c 코드 전반에서 64라는 값을 버퍼의 최대 값으로 설정하는 함수들을 벤치마킹했습니다.

Copy link
Contributor

Choose a reason for hiding this comment

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

64자로 지정하면 숫자 문자열을 63자까지만 처리하다는 의미죠 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

네, 맞습니다. 검토 결과, 뒤에 '\0'을 넣을 공간을 확보하기 위해 number_buffer_size를 사용하는 모든 부분에서 -1을 적용해야 한다는 점을 깨달았습니다. 꼼꼼한 리뷰에 감사드립니다.

const int max_vector_size = 2000;
float float_array[max_vector_size];
Comment on lines +5017 to +5018
Copy link
Contributor Author

Choose a reason for hiding this comment

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

float 배열의 최대 크기를 2000 으로 설정했습니다.

DB_SET *vec = NULL;
DB_VALUE e_val;
INTL_CODESET codeset = db_get_string_codeset (src);

if (p == NULL)
{
return ER_FAILED;
}

// Skip leading spaces and opening bracket
p = (char *) intl_skip_spaces (p, end, codeset);
if (p >= end || *p != '[')
{
return ER_FAILED;
}
p++;

while (p < end && count < max_vector_size)
{
// Skip spaces before number
p = (char *) intl_skip_spaces (p, end, codeset);
if (p >= end)
{
return ER_FAILED;
}

// Check for closing bracket
if (*p == ']')
{
break;
}

// Get number into buffer
buffer_idx = 0;
while (p < end && *p != ',' && *p != ']' && buffer_idx < number_buffer_size - 1)
{
if (!isspace (*p))
{
number_buffer[buffer_idx++] = *p;
}
p++;
}

if (buffer_idx == 0 || buffer_idx >= number_buffer_size - 1)
{

return ER_FAILED;
}

number_buffer[buffer_idx] = '\0';

// Convert to float
char *end_ptr = NULL;
errno = 0;
float_array[count] = strtof (number_buffer, &end_ptr);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

큐브리드는 bison에서 strtof, strtod 를 통해 파싱하고 있습니다.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

strtof 의 스펙 덕분에 자연그럽게 아래 쿼리도 동작합니다.

insert into vt (vec) values('[1.234e-5, 1.234e5, 1.234e10]');
insert into vt (vec) values('[1E-10, 1E10, 1E-3]');

확인:

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

int main() {
  const char *str1 = "3.14e2";   // Scientific notation
  const char *str2 = "2.718E-1"; // Scientific notation with uppercase E

  char *end;

  double num1 = strtod(str1, &end);
  float num2 = strtof(str2, &end);

  printf("strtod: %f\n", num1); // Outputs 314.000000
  printf("strtof: %f\n", num2); // Outputs 0.271800

  return 0;
}


if (errno == ERANGE)
{
return ER_FAILED;
}

if (*end_ptr != '\0')
{
return ER_FAILED;
}

count++;

// Skip spaces after number
p = (char *) intl_skip_spaces (p, end, codeset);
if (p >= end)
{
return ER_FAILED;
}

// Must be comma or closing bracket
if (*p == ']')
{
break;
}
else if (*p != ',')
{
return ER_FAILED;
}
p++;
}

// Check for closing bracket
if (p >= end || *p != ']')
{
return ER_FAILED;
}
p++;

// Skip trailing spaces
p = (char *) intl_skip_spaces (p, end, codeset);
if (p != end)
{
return ER_FAILED;
}

if (count == 0)
{
return ER_FAILED;
}

// Create vector and populate it
vec = db_vec_create (NULL, NULL, 0);
if (vec == NULL)
{
assert (er_errid () != NO_ERROR);
return er_errid ();
}

db_make_vector (result, vec);

for (int i = 0; i < count; ++i)
{
db_make_float (&e_val, float_array[i]);
if (db_seq_put (db_get_set (result), i, &e_val) != NO_ERROR)
{
return ER_FAILED;
}
}

return NO_ERROR;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

내부적으로 Set (Sequence)로 구현된 벡터의 틀을 우선 만들고,

db_make_vector(): db_set을 DB_VALUE화 한다.
db_make_float() : c 언어의 float을 DB_VALUE화한다.
db_seq_put (db_get_set ( ... )) : DB_VALUE가 된 Float을 Vector에 삽입합니다.

해당 코드는,

INSERT INTO st VALUES ({1.1, 2.2, 3.3});

실행 스택을 벤치마킹하였습니다.


/*
* tp_atobi - Coerce a string to a bigint.
* return: NO_ERROR or error code
Expand Down Expand Up @@ -9125,6 +9274,24 @@ tp_value_cast_internal (const DB_VALUE * src, DB_VALUE * dest, const TP_DOMAIN *
}
break;

case DB_TYPE_VECTOR:
switch (original_type)
{
case DB_TYPE_CHAR:
case DB_TYPE_VARCHAR:
case DB_TYPE_NCHAR:
case DB_TYPE_VARNCHAR:
{
Comment on lines +9182 to +9186
Copy link
Contributor Author

@vimkim vimkim Jan 6, 2025

Choose a reason for hiding this comment

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

이 부분에 대한 확신이 없습니다. 죄송합니다. 면밀히 리뷰 부탁드립니다.

현재 테스트 케이스는 DB_TYPE_CHAR 만으로도 통과하고 있습니다. 다른 테이블의 varchar 값을 vector로 변환하는 과정에서 나머지 3개의 case들이 필요할 것으로 추정하고 있습니다.

Copy link
Contributor

Choose a reason for hiding this comment

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

DB_TYPE_NCHAR와 DB_TYPE_VARNCHAR는 제거하는 것은 어떨까요 ?


err = tp_str_to_vector (src, target);
break;

}
default:
status = DOMAIN_INCOMPATIBLE;
break;
}
break;
case DB_TYPE_VOBJ:
if (original_type == DB_TYPE_VOBJ)
{
Expand Down