feat: add string type

[dshaaban01]

Added string types + string attribute types.

This is a stacked PR, please only review latest commit (15d2e21)

@ingomueller-net

[dshaaban01]

Note: 2 missing test files, have weird failures I will analyze later today.

[dshaaban01]

Note: 2 missing test files, have weird failures I will analyze later today.

The conclusion of my analysis is that the round tripping of the string attribute tests fail because it does not recognize !substrait.string as a type and recognizes is as none instead. I have therefore rebased this PR on top of the Substrait_Attr Base class PR so I can write my own custom string attr base class (currently I use MLIR built in string attr).

My new String Attribute is called StrAttr to disambiguate from MLIR's StringAttr.

[ingomueller-net]

LGTM, modulo the question about the built-in string attribute.

[ingomueller-net]

Note that there is a StringAttr in the built-in dialect. I don't understand all subtleties about using it but if we don't use it, it should be a conscious decision. Did you look at it?

[dshaaban01]

Yes, I originally was using the StringAttr (built-in type) but was getting failing tests on the literals.

"The conclusion of my analysis is that the round tripping of the string attribute tests fail because it does not recognize !substrait.string as a type and recognizes is as none instead. I have therefore rebased this PR on top of the Substrait_Attr Base class PR so I can write my own custom string attr base class (currently I use MLIR built in string attr).

My new String Attribute is called StrAttr to disambiguate from MLIR's StringAttr."

That's why I created this custom type.

[ingomueller-net]

I would like to understand this in more detail. Do you happen to have an old version of the code where the problem occurred? Does this do what you wanted to achieve? It yes, it's probably a good starting point for doing the same.

[dshaaban01]

Originally I was using the builtin StrAttr and wrote this test case.

// RUN: substrait-translate -substrait-to-protobuf %s \
// RUN: | FileCheck %s

// RUN: substrait-translate -substrait-to-protobuf %s \
// RUN: | substrait-translate -protobuf-to-substrait \
// RUN: | substrait-translate -substrait-to-protobuf \
// RUN: | FileCheck %s

// CHECK-LABEL: relations {
// CHECK-NEXT:    rel {
// CHECK-NEXT:      project {
// CHECK-NEXT:        common {
// CHECK-NEXT:          direct {
// CHECK-NEXT:          }
// CHECK-NEXT:        }
// CHECK-NEXT:        input {
// CHECK-NEXT:          read {
// CHECK:             expressions {
// CHECK-NEXT:          literal {
// CHECK-NEXT:            string: "hi"

substrait.plan version 0 : 42 : 1 {
  relation {
    %0 = named_table @t1 as ["a"] : tuple<si1>
    %1 = project %0 : tuple<si1> -> tuple<si1, !substrait.string> {
    ^bb0(%arg : tuple<si1>):
      %hi = literal "hi" : !substrait.string
      yield %hi : !substrait.string
    }
    yield %1 : tuple<si1, !substrait.string> 
  }
}

This test was failing and the problem I was getting was that it was inferring the return type of the literal Op as none, and subsequently the project op as tuple(s1, none) was the note/error I was getting.

[ingomueller-net]

I think I figured it out, see #55.

[ingomueller-net]

Suggested change

	// Substrait types
	// Substrait attributes

[ingomueller-net]

Nit: final new line.

[dshaaban01]

@ingomueller-net
I implemented your feedback from #55 and it worked! Genius! Thanks so much for figuring it out :)

[ingomueller-net]

Nice!! 🎉