Skip to content

Remove ISchema in FakeSchema #1872

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Dec 13, 2018
Merged

Conversation

wschin
Copy link
Member

@wschin wschin commented Dec 13, 2018

FakeSchema is not longer an ISchema and renamed to FakeSchemaFactory. It's a part of #1501.

@wschin wschin self-assigned this Dec 13, 2018
@wschin wschin force-pushed the fakeschema-not-ischema branch from ea00d32 to 114b59a Compare December 13, 2018 17:22
/// <summary>
/// A fake schema that is manufactured out of a SchemaShape.
/// It will pretend that all vector sizes are equal to 10, all key value counts are equal to 10,
/// and all values are defaults (for metadata).
/// </summary>
internal sealed class FakeSchema : ISchema
internal sealed class FakeSchemaFactory
Copy link
Contributor

@TomFinley TomFinley Dec 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FakeSchemaFactory [](start = 26, length = 17)

So @wschin, could we even just do away with most of this code altogether, and just have this be a static class with a single static method? I'm not sure what purpose creating this object actually serves, since you don't actually do anything with the instance per se. So why keep the type, when we can just nuke it?

I'm not even sure if there's some set of utilities for doing stuff with SchemaShape that we could exploit and just put it there.

Copy link
Member

@sfilipi sfilipi Dec 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe Schema definition? that has another Create method, creates from a type.


In reply to: 241493306 [](ancestors = 241493306)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a fully-static class now. On the other hand, I don't see another utility function creating Schema out of SchemaShape, so let's keep it for now.


In reply to: 241493306 [](ancestors = 241493306)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes -- though creating a SchemaShape out of a Schema is a generally useful utility, the utility of making "fake" schemas out of SchemaShape is somewhat limited, and I think probably something that will eventually go away as we convert things away from the old IDataLoader and IDataTransform idioms.

Copy link
Contributor

@TomFinley TomFinley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @wschin , just a few things to consider to simplify further, since I don't see that there's any need for this type to really exist at all, but since it's internal I'm not sure it's such a bit deal. (Just in case you wanted to do something with this in your copious spare time. 😄 )

return null;
return MakeColumnType(metaColumn);
ValueGetter<TValue> getter = (ref TValue value) => value = default;
return getter;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can they be defined inside the method? they are only called once.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so, not while having Utils.MarshalInvoke work (which is a very good thing).

Copy link
Member Author

@wschin wschin Dec 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I actually tried at least 5 different combinations and googled for several hours.

More specifically, as we need a generic-typed function to invoke, we need to have seperated functions for appending Type parameter. It's possible to have things like

Func<Delegate> act = () => (ValueGetter<VBuffer<TValue>>) ((ref VBuffer<TValue> value) => value = new VBuffer<TValue>(AllVectorSizes, 0, null, null));

in

        public static Schema Create<TValue>(SchemaShape shape)
        {
            var builder = new SchemaBuilder();

            for (int i = 0; i < shape.Count; ++i)
            {
                var metaBuilder = new MetadataBuilder();
                var partialMetadata = shape[i].Metadata;
                for (int j = 0; j < partialMetadata.Count; ++j)
                {
                    var metaColumnType = MakeColumnType(partialMetadata[i]);

                    Delegate del;
                    if (metaColumnType.IsVector)
                    {
                        Func<Delegate> act = () => (ValueGetter<VBuffer<TValue>>) ((ref VBuffer<TValue> value) => value = new VBuffer<TValue>(AllVectorSizes, 0, null, null));
                        del = Utils.MarshalInvoke(act, metaColumnType.ItemType.RawType);
                    }
                    else
                        del = Utils.MarshalInvoke(GetDefaultGetter<int>, metaColumnType.RawType);
                    metaBuilder.Add(partialMetadata[j].Name, metaColumnType, del);
                }
                builder.AddColumn(shape[i].Name, MakeColumnType(shape[i]));
            }
            return builder.GetSchema();
        }

but the user now needs to do Create<int> instead of Create.


In reply to: 241524344 [](ancestors = 241524344)

Copy link
Member

@sfilipi sfilipi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@wschin wschin merged commit 616dca2 into dotnet:master Dec 13, 2018
@wschin wschin deleted the fakeschema-not-ischema branch December 13, 2018 21:45
@ghost ghost locked as resolved and limited conversation to collaborators Mar 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants