azaurus1 / terraform-provider-pinot

A terraform provider for Apache Pinot
https://registry.terraform.io/providers/azaurus1/pinot/latest
Mozilla Public License 2.0
8 stars 1 forks source link

Support the `singleValueField` property on dimension fields #56

Closed gbrlcustodio closed 2 months ago

gbrlcustodio commented 3 months ago

Hey @azaurus1, I appreciate the effort you've been putting into this provider. I noticed that while setting up a table schema, some field properties are not supported. I see that you've recently added support for the notNull attribute, which is great. However, our use case requires setting a dimension column as multi-valued. The Pinot schema documentation (https://docs.pinot.apache.org/configuration-reference/schema#dimensionfieldspec) describes the singleValueField property, which is a boolean indicating if the field is a single-valued or a multi-valued column. In our case, a multi-valued column is modeled as a list, where the order of the values is preserved and duplicate values are allowed.

I'm willing to contribute to this if you're open to it. If so, I'm curious whether your intention is to have a single shared struct to define field properties or if you'd prefer to define specific structs for each type of field spec. For example:

type FieldSpec struct {
    Name        string `json:"name"`
    DataType    string `json:"dataType"`
    NotNull     bool   `json:"notNull,omitempty"`
    Format      string `json:"format,omitempty"`
    Granularity string `json:"granularity,omitempty"`
}

type Schema struct {
    SchemaName          string      `json:"schemaName"`
    DimensionFieldSpecs []FieldSpec `json:"dimensionFieldSpecs"`
    MetricFieldSpecs    []FieldSpec `json:"metricFieldSpecs,omitempty"`
    DateTimeFieldSpecs  []FieldSpec `json:"dateTimeFieldSpecs,omitempty"`
    PrimaryKeyColumns   []string    `json:"primaryKeyColumns,omitempty"`
}

If you have any guidance on how to proceed with adding extra properties, I'm ready to work on it.

azaurus1 commented 3 months ago

Hi @gbrlcustodio, we certainly welcome any contributions, in terms of the intentions for the structs: Yes using a shared struct would be ideal, and in go-pinot-api that is the current implementation (which I think the snippet is from)

I recognise maybe there's a bit of a disparity in terms of the fields between the go-pinot-api schema model and the terraform provider schema resource. (@hendoxc might be able to clarify that)

For adding this field, I would suggest: Make your changes in a fork of go-pinot-api

  1. include a demo that works for your use-case in the examples folder (ideally this interacts with the environment deployed by the docker-compose.yaml in the root)
  2. Include some unit-tests or Update the existing schema unit test to include the new property, up to you

Then once that's done make the PR for go-pinot-api

Note: The CreateSchema function in go-pinot-api will be changed in future to match the CreateSchemaFromBytes implementation, so try to use CreateSchemaFromBytes in your demo.

Once its implemented in go-pinot-api, its the same process for the terraform provider

Let me know if you have any other questions or issues