metesynnada commented on code in PR #5606:
URL: https://github.com/apache/arrow-datafusion/pull/5606#discussion_r1138667303
##########
datafusion/core/src/datasource/memory.rs:
##########
@@ -170,13 +170,6 @@ impl TableProvider for MemTable {
// Create a physical plan from the logical plan.
let plan = state.create_physical_plan(input).await?;
- // Check that the schema of the plan matches the schema of this table.
Review Comment:
You should not delete this piece of code. Since you deleted it, you will not
capture the errors if you change
https://github.com/apache/arrow-datafusion/blob/23814092bdd80533a5a673a5076d21125c8ef1e3/datafusion/core/tests/sqllogictests/test_files/ddl.slt#L442-L444
into
```
# Should create an empty table
statement ok
CREATE OR REPLACE TABLE table_without_values(field1 BIGINT, field2 BIGINT);
```
which is the default behavior of PostgreSQL.
##########
datafusion/core/tests/sqllogictests/test_files/ddl.slt:
##########
@@ -553,3 +553,15 @@ set datafusion.sql_parser.parse_float_as_decimal = false;
statement ok
set datafusion.sql_parser.enable_ident_normalization = true;
+
+
+statement ok
+create table foo(x int);
+
+statement ok
+insert into foo values (null);
+
+query I
+select * from foo;
+----
+NULL
Review Comment:
Cool 👍🏻
##########
datafusion/sql/src/statement.rs:
##########
@@ -127,42 +129,66 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
&& table_properties.is_empty()
&& with_options.is_empty() =>
{
- let plan = self.query_to_plan(*query, planner_context)?;
- let input_schema = plan.schema();
-
- let plan = if !columns.is_empty() {
- let schema =
self.build_schema(columns)?.to_dfschema_ref()?;
- if schema.fields().len() != input_schema.fields().len() {
- return Err(DataFusionError::Plan(format!(
+ match query {
+ Some(query) => {
+ let plan = self.query_to_plan(*query,
planner_context)?;
+ let input_schema = plan.schema();
+
+ let plan = if !columns.is_empty() {
+ let schema =
self.build_schema(columns)?.to_dfschema_ref()?;
Review Comment:
The problem of the `NOT NULL` default is still here since you did not change
the `self.build_schema(columns)`.
This should be the default behaviour:
```rust
pub fn build_schema(&self, columns: Vec<SQLColumnDef>) -> Result<Schema> {
let mut fields = Vec::with_capacity(columns.len());
for column in columns {
let data_type =
self.convert_simple_data_type(&column.data_type)?;
let prevent_null = column
.options
.iter()
.any(|x| x.option == ColumnOption::NotNull);
fields.push(Field::new(
normalize_ident(column.name,
self.options.enable_ident_normalization),
data_type,
!prevent_null,
));
}
Ok(Schema::new(fields))
}
```
then, it would help if you changed the `sqllogictests`, for example, from
```
describe table_with_normalization
----
field1 Int64 NO
field2 Int64 NO
```
to
```
describe table_with_normalization
----
field1 Int64 YES
field2 Int64 YES
```
In this way, you can give the same defaults with PostgreSQL.
##########
datafusion/core/src/datasource/memory.rs:
##########
@@ -170,13 +170,6 @@ impl TableProvider for MemTable {
// Create a physical plan from the logical plan.
let plan = state.create_physical_plan(input).await?;
- // Check that the schema of the plan matches the schema of this table.
- if !plan.schema().eq(&self.schema) {
- return Err(DataFusionError::Plan(
Review Comment:
`DataFusionError::Plan` can be converted to `DataFusionError::Internal` as
well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
