Skip to content

Commit 3bcd53b

Browse files
committed
[#220] fix: address CodeRabbit review findings
- create_table: 정리 경로를 FileSystem 트레이트로 추상화 (remove_dir_all을 FileSystem에 추가, tokio::fs 직접 호출 제거) - parser: PRIMARY KEY 목록의 쉼표 문법 검증 ((,a), (a,), (a,,b), (a b) 거부 — 식별자/쉼표 교대 강제) - parser: 테이블 레벨 PK 경로에도 문장 종료 검증 적용 (PK 뒤 이상한 토큰 시 에러, 세미콜론 없는 EOF는 허용) - e2e 테스트: 동기 Path::exists() 제거 (비동기 remove_dir_all로 정리) - 신규 파서 테스트 3개 (쉼표 문법/후행 토큰/EOF 종료)
1 parent 68f39fc commit 3bcd53b

5 files changed

Lines changed: 122 additions & 10 deletions

File tree

src/common/fs.rs

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,9 @@ pub trait FileSystem {
2020
/// 파일의 크기(bytes)를 반환합니다. (#265)
2121
/// `read_segment_rows`가 파일 전체를 메모리로 읽기 전에 예산을 확보하는 데 사용합니다.
2222
async fn metadata(&self, path: &Path) -> io::Result<u64>;
23+
/// 디렉토리와 그 내용 전체를 재귀적으로 삭제합니다. (#220)
24+
/// create_table 실패 시 생성 중인 테이블 디렉토리를 정리하는 데 사용합니다.
25+
async fn remove_dir_all(&self, path: &Path) -> io::Result<()>;
2326
}
2427

2528
pub struct RealFileSystem;
@@ -61,4 +64,8 @@ impl FileSystem for RealFileSystem {
6164
let metadata = tokio::fs::metadata(path).await?;
6265
Ok(metadata.len())
6366
}
67+
68+
async fn remove_dir_all(&self, path: &Path) -> io::Result<()> {
69+
tokio::fs::remove_dir_all(path).await
70+
}
6471
}

src/engine/actions/ddl/create_table.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,7 @@ impl DBEngine {
7676

7777
if !primary_key_columns.is_empty() {
7878
if let Err(error) = self.ensure_indices_loaded().await {
79-
let _ = tokio::fs::remove_dir_all(&table_path).await;
79+
let _ = self.file_system.remove_dir_all(&table_path).await;
8080
return Err(error);
8181
}
8282

@@ -89,7 +89,7 @@ impl DBEngine {
8989
);
9090

9191
if let Err(error) = self.index_manager.create_index(meta).await {
92-
let _ = tokio::fs::remove_dir_all(&table_path).await;
92+
let _ = self.file_system.remove_dir_all(&table_path).await;
9393
return Err(error);
9494
}
9595
}

src/engine/actions/test_composite_pk_e2e.rs

Lines changed: 16 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -24,9 +24,9 @@ use crate::engine::{DBEngine, SharedWALManager};
2424
2525
async fn build_test_engine(test_name: &str) -> (DBEngine, SharedWALManager) {
2626
let base_path = PathBuf::from("target/test_composite_pk").join(test_name);
27-
if base_path.exists() {
28-
tokio::fs::remove_dir_all(&base_path).await.unwrap();
29-
}
27+
28+
// 남은 이전 실행 정리 — remove_dir_all은 존재하지 않아도 실패를 신경 쓰지 않음
29+
let _ = tokio::fs::remove_dir_all(&base_path).await;
3030

3131
let config = LaunchConfig::default_for_base_path(&base_path);
3232
tokio::fs::create_dir_all(&config.data_directory)
@@ -139,7 +139,10 @@ async fn composite_pk_rejects_duplicate_key_combinations() {
139139
.await
140140
.expect("different combination with reused group_id must be accepted");
141141

142-
assert_eq!(engine.full_scan(memberships_table()).await.unwrap().len(), 3);
142+
assert_eq!(
143+
engine.full_scan(memberships_table()).await.unwrap().len(),
144+
3
145+
);
143146
}
144147

145148
#[tokio::test]
@@ -213,7 +216,11 @@ async fn composite_pk_lookup_is_correct_on_a_large_table() {
213216
)
214217
.await
215218
.unwrap();
216-
assert_eq!(result.rows.len(), 1, "user_id = 42 must match exactly one row");
219+
assert_eq!(
220+
result.rows.len(),
221+
1,
222+
"user_id = 42 must match exactly one row"
223+
);
217224
assert_eq!(result.rows[0].fields[0], ExecuteField::Integer(42 % 7));
218225

219226
// 존재하지 않는 키: 0행 (과거 버그로는 0행이 정답처럼 보였지만
@@ -296,7 +303,10 @@ async fn composite_pk_maintains_index_across_update_and_delete() {
296303
"update memberships set user_id = 2, group_id = 3 where user_id = 1 and group_id = 1;",
297304
)
298305
.await;
299-
assert!(conflict.is_err(), "duplicate combination via update must fail");
306+
assert!(
307+
conflict.is_err(),
308+
"duplicate combination via update must fail"
309+
);
300310

301311
// DELETE: 인덱스 항목 제거
302312
execute_sql(

src/engine/parser/implements/ddl/table.rs

Lines changed: 42 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,9 @@ impl Parser {
9090
}
9191

9292
let mut columns: Vec<String> = vec![];
93+
// 식별자와 쉼표가 교대로 와야 합니다 (#220):
94+
// PRIMARY KEY (a, b) O, (,a) / (a,) / (a,,b) X
95+
let mut expect_identifier = true;
9396

9497
loop {
9598
if !self.has_next_token() {
@@ -99,10 +102,32 @@ impl Parser {
99102
let current_token = self.get_next_token();
100103

101104
match current_token {
102-
Token::RightParentheses => break,
103-
Token::Comma => continue,
105+
Token::RightParentheses => {
106+
if expect_identifier && !columns.is_empty() {
107+
return Err(ParsingError::wrap(
108+
"trailing comma in 'PRIMARY KEY (...)'".to_string(),
109+
));
110+
}
111+
break;
112+
}
113+
Token::Comma => {
114+
if expect_identifier {
115+
return Err(ParsingError::wrap(
116+
"expected column name in 'PRIMARY KEY (...)'. but your input word is 'Comma'"
117+
.to_string(),
118+
));
119+
}
120+
expect_identifier = true;
121+
}
104122
Token::Identifier(column_name) => {
123+
if !expect_identifier {
124+
return Err(ParsingError::wrap(format!(
125+
"expected ',' or ')' after column name in 'PRIMARY KEY (...)'. but your input word is '{:?}'",
126+
column_name
127+
)));
128+
}
105129
columns.push(column_name);
130+
expect_identifier = false;
106131
}
107132
_ => {
108133
return Err(ParsingError::wrap(format!(
@@ -172,6 +197,21 @@ impl Parser {
172197
}
173198
}
174199

200+
// 문장 종료 검증은 일반 경로와 동일하게 적용합니다 (#220):
201+
// 테이블 레벨 PK 뒤에 이상한 토큰이 남으면 에러.
202+
if !self.has_next_token() {
203+
return Ok(query);
204+
}
205+
206+
let current_token = self.get_next_token();
207+
208+
if Token::SemiColon != current_token {
209+
return Err(ParsingError::wrap(format!(
210+
"expected ';'. but your input word is '{:?}'",
211+
current_token
212+
)));
213+
}
214+
175215
return Ok(query);
176216
}
177217

src/engine/parser/test/create_table.rs

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -156,6 +156,61 @@ pub fn create_table_rejects_primary_key_referencing_unknown_column() {
156156
);
157157
}
158158

159+
/// PK 목록의 문법 오류(쉼표 위치)를 거부해야 합니다 (#220, CodeRabbit)
160+
#[test]
161+
pub fn create_table_rejects_primary_key_comma_syntax_errors() {
162+
let cases = [
163+
"CREATE TABLE t (a INTEGER, PRIMARY KEY (,a));", // 선두 쉼표
164+
"CREATE TABLE t (a INTEGER, PRIMARY KEY (a,));", // 후행 쉼표
165+
"CREATE TABLE t (a INTEGER, PRIMARY KEY (a,,b));", // 연속 쉼표
166+
"CREATE TABLE t (a INTEGER, PRIMARY KEY (a b));", // 쉼표 누락
167+
];
168+
169+
for text in cases {
170+
let mut parser = Parser::with_string(text.to_owned()).unwrap();
171+
assert!(
172+
parser.parse(ParserContext::default()).is_err(),
173+
"should reject: {}",
174+
text
175+
);
176+
}
177+
}
178+
179+
/// 테이블 레벨 PK 뒤에 이상한 토큰이 남으면 에러 (#220, CodeRabbit)
180+
#[test]
181+
pub fn create_table_rejects_trailing_tokens_after_table_level_primary_key() {
182+
let text = r#"
183+
CREATE TABLE "test_db".t
184+
(
185+
a INTEGER,
186+
PRIMARY KEY (a)
187+
) unexpected;
188+
"#
189+
.to_owned();
190+
191+
let mut parser = Parser::with_string(text).unwrap();
192+
193+
assert!(parser.parse(ParserContext::default()).is_err());
194+
}
195+
196+
/// 테이블 레벨 PK + 세미콜론 없는 종료(EOF)는 정상 파싱 (#220)
197+
#[test]
198+
pub fn create_table_accepts_table_level_primary_key_without_semicolon() {
199+
let text = r#"
200+
CREATE TABLE "test_db".t
201+
(
202+
a INTEGER,
203+
PRIMARY KEY (a)
204+
)
205+
"#
206+
.to_owned();
207+
208+
let mut parser = Parser::with_string(text).unwrap();
209+
210+
let statements = parser.parse(ParserContext::default()).unwrap();
211+
assert_eq!(statements.len(), 1);
212+
}
213+
159214
#[test]
160215
pub fn create_table() {
161216
let text = r#"

0 commit comments

Comments
 (0)