alamb commented on code in PR #7755:
URL: https://github.com/apache/arrow-datafusion/pull/7755#discussion_r1349509095
##########
datafusion/common/src/functional_dependencies.rs:
##########
@@ -43,13 +46,15 @@ pub struct Constraints {
impl Constraints {
/// Create empty constraints
pub fn empty() -> Self {
- Constraints::new(vec![])
+ Constraints::new_private(vec![])
}
- // This method is private.
- // Outside callers can either create empty constraint using
`Constraints::empty` API.
- // or create constraint from table constraints using
`Constraints::new_from_table_constraints` API.
- fn new(constraints: Vec<Constraint>) -> Self {
+ /// Create a new `Constraints` object from the given `constraints`.
+ /// Users should use the `empty` or `new_from_table_constraints` functions
+ /// for constructing `Constraints`. This constructor is for internal
+ /// purposes only and does not check whether the argument is valid. The
user
+ /// is responsible for supplying a valid vector of `Constraint` objects.
+ pub fn new_private(constraints: Vec<Constraint>) -> Self {
Review Comment:
Another name for this might be `new_unverified` that emphasized what was
different (rather than it should be used)
##########
datafusion/core/src/datasource/listing/table.rs:
##########
@@ -23,19 +23,19 @@ use std::{any::Any, sync::Arc};
use super::helpers::{expr_applicable_for_cols, pruned_partition_list,
split_files};
use super::PartitionedFile;
-use crate::datasource::file_format::file_compression_type::{
- FileCompressionType, FileTypeExt,
-};
-use crate::datasource::physical_plan::{
- is_plan_streaming, FileScanConfig, FileSinkConfig,
-};
use crate::datasource::{
file_format::{
Review Comment:
thank you for cleaning up the imports
##########
datafusion/sql/src/statement.rs:
##########
@@ -681,8 +680,22 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
order_exprs,
unbounded,
options,
+ constraints,
} = statement;
+ let mut constraints = constraints;
+ for column in &columns {
+ for option in &column.options {
+ if let ast::ColumnOption::Unique { is_primary } =
option.option {
+ constraints.push(ast::TableConstraint::Unique {
+ name: None,
+ columns: vec![column.name.clone()],
+ is_primary,
+ })
+ }
+ }
Review Comment:
My reading of this code is that other types of constraints will be silently
ignored. Perhaps we can error if we see an unknown constraint?
In fact, I think if this code called
```
let constraints =
Constraints::new_from_table_constraints(&constraints,
&df_schema)?;
```
It would return errors for unsupported constraints already
##########
datafusion/proto/src/logical_plan/to_proto.rs:
##########
@@ -1623,6 +1625,35 @@ impl From<JoinConstraint> for protobuf::JoinConstraint {
}
}
+impl From<Constraints> for protobuf::Constraints {
Review Comment:
I didn't see a test for this code, perhaps you can add one to
https://github.com/apache/arrow-datafusion/blob/main/datafusion/proto/tests/cases/roundtrip_logical_plan.rs
so we have some basic coverage
--
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]
